Revise notification flows #5827
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grzesiek2010
requested changes
Nov 16, 2023
...va/org/odk/collect/android/notifications/builders/FormUpdatesAvailableNotificationBuilder.kt
Show resolved
Hide resolved
...roidTest/java/org/odk/collect/android/feature/formmanagement/PreviouslyDownloadedOnlyTest.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/notifications/NotificationUtils.kt
Outdated
Show resolved
Hide resolved
...in/java/org/odk/collect/android/notifications/builders/FormsSubmissionNotificationBuilder.kt
Outdated
Show resolved
Hide resolved
| @@ -66,8 +66,7 @@ class AutoSendTest { | |||
|
|
|||
| testDependencies.scheduler.runDeferredTasks() | |||
|
|
|||
| mainMenuPage | |||
| .clickViewSentForm(1) | |||
| mainMenuPage.clickViewSentForm(1) | |||
There was a problem hiding this comment.
What's the point of checking this here if we do not check the status (success/failed)? No matter if sending failed or not the form will be there.
There was a problem hiding this comment.
It's still checking that the form moves to "Sent" which is valuable, but I see your point that it doesn't check that the form ends up with the correct status. I'll have a look into if this is tested elsewhere, and if not add coverage here.
There was a problem hiding this comment.
A bunch of other tests fail if I mess with code that updates instance statuses after sending so I think this is ok for now.
..._app/src/androidTest/java/org/odk/collect/android/feature/instancemanagement/AutoSendTest.kt
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/NotificationDrawer.kt
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/NotificationDrawer.kt
Outdated
Show resolved
Hide resolved
collect_app/src/androidTest/java/org/odk/collect/android/support/pages/NotificationDrawer.kt
Show resolved
Hide resolved
seadowg
force-pushed
the
fix-notifications
branch
from
4949add
to
72e5ea5
Compare
grzesiek2010
requested changes
Nov 21, 2023
collect_app/src/main/java/org/odk/collect/android/utilities/ApplicationConstants.java
Show resolved
Hide resolved
seadowg
force-pushed
the
fix-notifications
branch
from
95a058e
to
08e16f1
Compare
grzesiek2010
approved these changes
Nov 22, 2023
|
Tested with Success! Verified on Androids: 10 Verified cases:
|
|
Tested with Success! Verified on Androids: 13 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5811
The solution here is to make all (event) notifications follow these rules:
Why is this the best possible solution? Were any other approaches considered?
For each case, there are probably better solutions, but for now we want to get on solid ground and avoid any weird problems from notification flows (like that encountered in the issue). The biggest sacrifice we're making here is that we're removing ways for users to fix problems that the notifications are flagging (like failed match exactly syncs). However, these were half baked anyway as they would only work if the user was currently in the correct project and could cause issues if they were midway through form entry.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Should only change the behaviour of notifications. It'd be good to try and find if the new flows allow any weird navigation scenarios.
Before submitting this PR, please make sure you have:
./gradlew checkAlland confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTestlocally.