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.

Sign up for GitHub

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

Jump to bottom

Minimum browser requirements for cht-core 4 #8159

Closed
kennsippell opened this issue Mar 30, 2023 · 6 comments
Closed

Minimum browser requirements for cht-core 4 #8159

kennsippell opened this issue Mar 30, 2023 · 6 comments
Assignees
@lorerod
Labels
Type: Bug Fix something that isn't working as intended

Comments

@kennsippell
Copy link
Member

kennsippell commented Mar 30, 2023 •
edited by garethbowen
Loading

We should fail loudly if the minimum browser requirement is not met. In the same way we are clear on the minimum android os version, we should be clear on the minimum chrome version (and other requirements).

Originally posted by @derickl in #8143 (comment)
@kennsippell kennsippell added the Type: Bug Fix something that isn't working as intended label Mar 30, 2023
@garethbowen
Copy link
Member

I think failing loudly is a good idea. If we know the app will not work (as in the linked issue) then the user should not be able to log in, or if they are logged in, the app should be locked. The idea would be to make it so obvious to whoever is setting up the device that it's caught early before the phone is given to the CHW.

We could also have a warning stage for if the browser is quite old but technically still supported, but I doubt this will prompt any action from the users so it's likely just nagging. This would be an interesting experiment to run to see how often this prompts user action, but probably outside the scope of this issue.

The reason this isn't a quick win is because currently the testing is all done on recent browsers - this is a technical limitation of our test setup not by design. So if we officially support Chrome X then we need to fully test Chrome X before release. This is a good thing, but it's worth highlighting the current state.

Steps I can see for the implementation of this...

  1. Decide on what the minimum browser version should be, which is more permissive than the current documented support. At the same time decide whether the minimum will be a rolling version (last x years of browser), or whether it can only be changed in a breaking version requiring a major release.
  2. Work out how to run our e2e test suite on that version
  3. Change the current warning to block login completely, and add an in-app screen to block using the app (may be a duplicate of Browser compatibility modal notice for Chrome version 75-90 #7770 )

@kennsippell
Copy link
Member Author

whether it can only be changed in a breaking version requiring a major release

Yes please! This will really maintain clarity in our upgrade process.

@kennsippell kennsippell mentioned this issue Mar 31, 2023
Merged
@lorerod lorerod self-assigned this Aug 7, 2023
@mrjones-plip mrjones-plip mentioned this issue Aug 16, 2023
Closed
@garethbowen
Copy link
Member

In #8456 it became clear that we need to run browser checking code before attempting to load the app as the Android app bypasses the login page altogether and if the main bundle crashes on an older browser the user is stuck with no useful info.

garethbowen added a commit that referenced this issue Aug 22, 2023
@garethbowen
chore(#8159): update browserslist to chrome 90
a237de0
@garethbowen garethbowen mentioned this issue Aug 22, 2023
Merged
5 tasks
garethbowen added a commit that referenced this issue Aug 24, 2023
@garethbowen
chore(#8159): update browserslist to chrome 90
a0d5876
@m5r m5r mentioned this issue Sep 13, 2023
Closed
@garethbowen garethbowen mentioned this issue Oct 3, 2023
Closed
5 tasks
@lorerod lorerod mentioned this issue Oct 24, 2023
Merged
5 tasks
lorerod added a commit that referenced this issue Nov 8, 2023
@lorerod @garethbowen
chore(#8159): Run our e2e standard test on minimum browser requiremen…
1d9e77b 
…ts (#8663)

* Fix standard config specs to run on chrome 90

* Add changes after npm install

* Revert unwanted changes

* Add suggested feedback

* Fix chrome_version env variable setup

* Add chrome version to test name

* Update binary not to use my local chrome

* install chromedriver 90

* Revert deletion of chromedriver 90 dependency

* Delete chromedriver dependency

* Fix spaces in run

* Change where to set CHROME_VERSION env

* Add no-sandbox to prevent session not created chromedriver error

* Revert change in the test names

* Merge Install Chrome and Chromedriver version 90 in one step

Co-authored-by: Gareth Bowen <gareth@medic.org>

* Change name in step and, simplify chrome args

* Fix name calculation

* Fix eslint

* Fix >> character

* Add await browser.url('/'); before login

* Add await browser.url('/'); before logout

* Change roleField selector to work also on Chrome90

* Fix eslint

* Use browserVersion to choose the method to get text from a span element

* Fix eslint

* Delete browser.url('/'); after logout

* Delete no-sandbox option

* Delete unnecessary waits

* expose a common function to validate if the browser version is the minimum

* Fix eslint

* Create standard enketo wdio page to have common enketo forms definitions

* Fix eslint

* Delete BrowserVersion as is not neccesary if setting binary

* Set undefined browserVersion when is not minimum

* Delete chromedriver dependencies

* Add chromedriver binary path

* Add disable-dev-shm-usage arg

* Fix eslint

* Fix misspelled

* Check chromedriver version

* Get chromedriver version

* Use node modules chromedriver

* Fix driver version output

* Add no-sandbox to make the test pass, and print browser and driver versions

* Fix eslint

* Add no-sandbox argument only when running tests with chrome90

* Implement feedback

---------

Co-authored-by: Maria Lorena Rodriguez Viruel <marialorenarodriguezviruel@MacBook-Pro-de-Maria.local>
Co-authored-by: Gareth Bowen <gareth@medic.org>
@jkuester
Copy link
Contributor

@lorerod should this issue be closed now because of #8663?

@lorerod
Copy link
Contributor

lorerod commented Mar 19, 2024

@jkuester I'm using this issue as a placeholder.
#8663 only runs standard config e2e tests on the minimum browser version. But then the standard config was deleted.
The next step is to resume this work and run default config e2e tests on the minimum browser version. I will create a separate PR for that work.

@lorerod lorerod mentioned this issue Mar 26, 2024
Closed
This was referenced Oct 4, 2024
Open
Open
Open
Open
@lorerod
Copy link
Contributor

lorerod commented Oct 4, 2024 •
edited
Loading

I'm closing this issue because I've split the minimum browser requirements work into smaller, more focused issues. Each issue will address a specific suite to ensure better tracking.

@lorerod lorerod closed this as completed Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lorerod lorerod

Labels
Type: Bug Fix something that isn't working as intended
Projects
Product Team Activities
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@garethbowen @jkuester @kennsippell @lorerod