Skip to content
This repository has been archived by the owner on Dec 26, 2022. It is now read-only.

feat: add support for first, second, and third injection independently #81

Merged
merged 4 commits into from
Nov 27, 2021

Conversation

hschaeidt
Copy link
Collaborator

The first and the second commit could be cherry-picked individually, as they are only bug fixes of the existing code.

  • 9a7b22c: fixes a bug where the autoBook status was not correctly read from the storage
  • 787ff96: fixes a bug where the automatic booking was no longer possible, due to an additional radio button in the confirmation popup
  • 5f4f17c: adds an advanced option for those who wish to choose single injections

@hschaeidt hschaeidt changed the title WIP: add support for first, second, and third injection independently add support for first, second, and third injection independently Jul 26, 2021
@hschaeidt
Copy link
Collaborator Author

hschaeidt commented Jul 26, 2021

There is still an issue, where the book script cannot proceed with the automatic reservation:

  • fireFullClick(popupConfirmation); in book.js L445

This seems to trigger a whole page reload, after that the script isn't able to resume where it was: selecting the master patient.

I believe this was already broken before because in the current version I am unable to even check the automatic reservation option. Will investigate into that later on, maybe. If anyone has any idea of how to fix this, you're welcome :)

@julienw
Copy link
Collaborator

julienw commented Jul 26, 2021

There is still an issue, where the book script cannot proceed with the automatic reservation:

* `fireFullClick(popupConfirmation);` in book.js L445

This seems to trigger a whole page reload, after that the script isn't able to resume where it was: selecting the master patient.

I believe this was already broken before because in the current version I am unable to even check the automatic reservation option. Will investigate into that later on, maybe. If anyone has any idea of how to fix this, you're welcome :)

Yeah, this is from before. I filed #70 about that. I believe we'll need to run a different script after the page reload.

@hschaeidt hschaeidt changed the title add support for first, second, and third injection independently feat: add support for first, second, and third injection independently Jul 26, 2021
@hschaeidt
Copy link
Collaborator Author

hschaeidt commented Jul 26, 2021

Yeah, this is from before. I filed #70 about that. I believe we'll need to run a different script after the page reload.

Thank you for pointing out this issue.

If I understand it correctly I broke the current "wanted" behaviour with my bugfix. The script unintentionally exited early in book.js on line 439 (if (!popupConfirmation) {). The reason is, that with the addition of the radio buttons asking for the age of the user, before showing the confirmation button, the script always failed here.

In 787ff96 I fixed the popup handling and the submission of the confirmation button will now succeed again. The side effect is that the user will never have a notification because we no longer execute await browser.runtime.sendMessage book.js#L474 nor await browser.runtime.sendMessage book.js#L464 as the scripts exits early after the reload happening with fireFullClick(popupConfirmation) book.js#L445 and no more error is thrown because the popup confirmation succeeds.

Probably I should revert this fix for now. What do you think?

EDIT: Another temporary possible fix coming to my mind is to always throw right after fireFullClick(popupConfirmation). So that the browser notification is still sent to the user.

@dunglas
Copy link
Owner

dunglas commented Nov 25, 2021

@hschaeidt as you may have heard, we need this now in France :). Do you think that this PR is ready?

@hschaeidt
Copy link
Collaborator Author

It might need some rework depending on what changed on Doctolib's DOM side in the meantime and if #70 is still relevant in this context. I can probably afford to spend some time on it within the next week to clean it up and get it ready.

@dunglas
Copy link
Owner

dunglas commented Nov 25, 2021

IMHO we can merge even if #70 isn't fixed. (it still works for most use cases)

@hschaeidt
Copy link
Collaborator Author

Okay, it's working in a reliable way now with the last commit. I did some tests with vaccination centers, but not with doctors or pharmacies. However, issue #70 needs to be treated separately in my opinion. The topic is too big to be solved within this merge request.

The booking behaviour doesn't change in any case from main.

@dunglas
Copy link
Owner

dunglas commented Nov 27, 2021

Thanks you very much! I'll review. The code this afternoon and try to make a release.

@dunglas dunglas merged commit 3d1022d into dunglas:main Nov 27, 2021
@dunglas
Copy link
Owner

dunglas commented Nov 27, 2021

Thanks @hschaeidt!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants