Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for Reschedule api as well. For Users who wants to reschedule booking rather fresh booking for 1st dose. #170

Open
wants to merge 2 commits into
base: ext-dev/reschedule
Choose a base branch
from

Conversation

vsachinv
Copy link
Contributor

No description provided.

@catch-n-release
Copy link
Contributor

Hey, the current code works for rescheduling as is. You can try it.
This would just increase lines for a functionality that's already present.

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

@catch-n-release : current code does not provide rescheduling option

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

@vsachinv - Thank you for the PR! If I understand this correctly, to reschedule an appointment, a slot has to be available in the first place. So why not cancel and book?

@catch-n-release
Copy link
Contributor

catch-n-release commented May 20, 2021

@pallupz If rescheduling is booking a new appointment for a beneficiary already having one, then the current code does, even if it's not explicit functionality. It's tested.

@catch-n-release
Copy link
Contributor

catch-n-release commented May 20, 2021

@pallupz Please correct me if I have a wrong understanding.😅

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

@catch-n-release I agree that this may not be a necessary feature. I think the idea is to book another slot without having to go into the portal and canceling existing one manually.

@catch-n-release
Copy link
Contributor

@pallupz I totally agree. But one doesn't even have to cancel the appointment. It books a new appointment for the same beneficiary (if not vaccinated). I think you have underestimated the power of your own code 😆. I can cross-check and send my findings to you. But then again, your call totally. 😁

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

oh thats interesting.. I hadn't tested it that way. So if we have already have an appointment, and try to book another one, it essentially works like canceling and booking? I thought that might throw some error. Can't help but wonder what was the point for this reschedule endpoint then?!

@vsachinv
Copy link
Contributor Author

vsachinv commented May 20, 2021

@catch-n-release I have checked. it doesn't work if already schedule exist in the current code as it hits only schedule api not reschedule api. It fails. Due to this i couldn't book :( . Due that only i came up with changes :) .
@pallupz Looks like cancel scheduled slot is not working. Even portal has not given cancel option only rescheduling is there.

Check below response

================================= ATTEMPTING BOOKING ==================================================
Booking Response Code: 400
Booking Response : {"errorCode":"APPOIN0022","error":"An active first dose appointment already exists for the individual with reference id 33424062792331"}
Response: 400 : {"errorCode":"APPOIN0022","error":"An active first dose appointment already exists for the individual with reference id 33424062792331"}

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

@vsachinv - The portal does have cancel option but the design makes is super hard to find. For each beneficiary's section, who have an active appointment, there's a tiny 'x' button on the right top corner

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

Nonetheless, this sort of a serious change. Will merge to another branch (not main) and test in the weekend.

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

Could you please raise this to ext-dev/reschedule branch?

@vsachinv
Copy link
Contributor Author

@pallupz Sure and thanks i will check that cancel option as well.

@vsachinv vsachinv changed the base branch from main to ext-dev/reschedule May 20, 2021 07:08
@pallupz
Copy link
Owner

pallupz commented May 20, 2021

I have no idea why they designed it so. Should've gone with a proper red button with actual text.

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

I ran through the changes. Since the details needed are almost the same, what if instead of creating a separate function for rescheduling, we try to incorporate it into booking itself? That way we may be able to avoid a bunch of code duplication.

@vsachinv
Copy link
Contributor Author

@pallupz thanks. Now, I am able to cancel the slot from cowin. Yeah its not intuitive for cancel and also from Arogya Setu app on cancelling it was giving API error. Nevertheless thanks for helping me :). I have changed branch PR to ext-dev/reschedule as well.

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

I haven't gone through in detail so not sure if that'd be possible.

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

Essentially, if someone with active appointment tries to book, code will utilize Rescheduling endpoint. Otherwise, it will go with Booking endpoint.

@catch-n-release
Copy link
Contributor

@vsachinv That's strange! I got people to reschedule 3 days ago without facing that error. Could you tell me when did you last try rescheduling?
Anyway, If @pallupz thinks it's good I have no more questions/doubts about it.

@pallupz
Copy link
Owner

pallupz commented May 20, 2021

maybe @vsachinv is trying for second dose. I'm considering implementing it because the portal UI is not all intuitive.

@vsachinv
Copy link
Contributor Author

@pallupz No i was trying for first dose only just i didn't cancel existing appointments only. One of the appointment was of past date.

@vsachinv
Copy link
Contributor Author

@pallupz I didn't see any code of reschedule for already existing appointments. How it was using reschedule if appointment already exists? @catch-n-release I was trying on 19th May.

@pallupz pallupz mentioned this pull request May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants