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

fix: correct invalid stored preference for in-app notifications #27237

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

warrensplayer
Copy link
Contributor

@warrensplayer warrensplayer commented Jul 7, 2023

Additional details

When navigating to the settings page after enabling in-app notifications with the top banner, the value for the notifyWhenRunCompletes setting was not given a chance to load before saving off the value in a Vue ref. The HTML expected the value to be an array and the backend was trying to default the value to ['failed'], but it would end up null in this case. If the user attempted to click one of the checkboxes to set the value, then the code would cause the value to get set as a boolean true instead of an array of strings. Finally, if a notification attempted to be fired, or the app was closed and opened again, code that expected the value for notifyWhenRunCompletes to be an array would cause the app to crash.

The fix was to reactively set the value for notifyWhenRunCompletes in the NotifcationSettings component once it had loaded when navigating from the top banner. Also, a fix was added to LocalSettingsActions.refreshLocalSettings() to make sure to fix this preference value if it was set to a boolean.

Steps to test

Reproduce the original issue

  • When on the develop branch, open the state.json file under ~/Library/Application Support/Cypress/cy/production/projects/__global__ and remove the following values:
    • desktopNotificationsEnabled
    • notifyWhenRunCompletes
    • notifyWhenRunStartsFailing
    • notifyWhenRunStarts
  • Start Cypress in open mode and open the App from the Launchpad
  • Click the "Enable desktop notifications" button
  • The app will navigate to the "Desktop notifications" part of the Settings page
  • Confirm that the "Notify me when a run completes" section has nothing checked
  • Check one box and they will all show checked
  • Open the state.json file from above and notice that the notifyWhenRunCompletes value will be true
  • Close the Cypress app and reopen it
  • When the app tries to show the page for selecting the testing type, it will crash

Before fix

Screen.Recording.2023-07-07.at.2.17.49.PM.mov

Testing fix to repair bad value

  • Update to this branch
  • Open the app again
  • The app should not crash
  • Open the state.json file from above and notice that the notifyWhenRunCompletes value will be an array of strings
  • Navigate to the ""Desktop notifications" part of the Settings page and change the values for the "Notify me when a run completes" section
  • Confirm those values are saved in the state.json file

Testing new code without existing settings

  • Make sure app is closed
  • While on this branch, open the state.json file under ~/Library/Application Support/Cypress/cy/production/projects/__global__ and remove the same values as listed above
  • Open the app and a testing mode and click the "Enable desktop notifications" button
  • The app will navigate to the "Desktop notifications" part of the Settings page
  • Confirm that the "Notify me when a run completes" section has only failed checked as the default
  • Change the options under this section
  • Confirm those values are saved in the state.json file

After fix

Screen.Recording.2023-07-07.at.2.08.25.PM.mov

How has the user experience changed?

PR Tasks

@warrensplayer warrensplayer requested a review from a team July 7, 2023 18:20
@cypress
Copy link

cypress bot commented Jul 7, 2023

7 flaky tests on run #48654 ↗︎

0 5320 81 0 Flakiness 7

Details:

Merge branch 'develop' into stokes/27228_notification_preference_fix
Project: cypress Commit: 387208d4ca
Status: Passed Duration: 13:17 💡
Started: Jul 7, 2023 8:23 PM Ended: Jul 7, 2023 8:36 PM
Flakiness  commands/net_stubbing.cy.ts • 2 flaky tests • 5x-driver-chrome:beta

View Output Video

Test Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse Output Video
... > can timeout when retrieving upstream response Output Video
Flakiness  e2e/origin/commands/navigation.cy.ts • 1 flaky test • 5x-driver-chrome:beta

View Output Video

Test Artifacts
cy.origin navigation > #consoleProps > .go() Output Video
Flakiness  e2e/origin/config_env.cy.ts • 1 flaky test • 5x-driver-chrome:beta

View Output Video

Test Artifacts
cy.origin- Cypress.config() > serializable > overwrites different values in secondary, even if the Cypress.config() value does not exist in the primary Output Video
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-chrome:beta

View Output Video

Test Artifacts
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@marktnoonan
Copy link
Contributor

marktnoonan commented Jul 7, 2023

So far this seems to be working well when testing 👍 - going to try some more scenarios.

Noticed we link to https://docs.cypress.io/guides/cloud/cypress-app-integration#Troubleshooting for troubleshooting, but that URL is 404, if we need to change anything for that we should do it in this PR.

Copy link
Contributor

@mike-plummer mike-plummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test 😢 ?

const listRef = ref()

// allow for gql value to load when navigating straight here from EnableNotificationsBanner
watchEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC there's a vueuse/core function that effectively does this a bit more succinctly, but this gets the job done so I'm fine with it

cli/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jordanpowell88 jordanpowell88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good to me. Ideally the NotifyWhenRunCompletes would never be a type boolean in the first place but this PR certainly accounts for the scenario in which it is.

@marktnoonan
Copy link
Contributor

It looks like a PR has already been merged for the on-link that was 404, just hasn't been deployed yet, so no action needed in this branch to get it working.

@warrensplayer
Copy link
Contributor Author

@mike-plummer

No test 😢 ?

Added dcc13c0

Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, glad to have the test, it successfully repaired the state for me.

@warrensplayer warrensplayer merged commit a3e4a8a into develop Jul 7, 2023
76 of 78 checks passed
@warrensplayer warrensplayer deleted the stokes/27228_notification_preference_fix branch July 7, 2023 20:36
@wlsf82
Copy link

wlsf82 commented Jul 7, 2023

Do you have a date for the fix release?

@lmiller1990
Copy link
Contributor

First thing next week (July 10)

Sorry, thanks for the interest @wlsf82 🎉

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 10, 2023

Released in 12.17.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.17.1, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jul 10, 2023
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.

Cypress v12.17.0 crashes when notifications are enabled
6 participants