-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Show snackbar notification when a form is saved or sent #5609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we've got this to play with, I think there are a couple of behavioural things we should change before diving into a code review:
- The snackbar should be hidden after an action (like "View") is clicked. Right now it's on-screen for a while (which is good), but if you click "View" and then press back to return the menu, the snack bar is still there and is confusing.
- The "It looks like you're offline..." doesn't really make sense as-is. We only show it if the device is connected to the network required for auto send, but somehow also offline which will happen very rarely (even if at all) in reality. I think we should show also show this message in the cases that auto send won't happen (auto send is wifi only, but we only have a cellular connection for instance). We might need to tweak the text a little as "offline" won't be strictly true any longer. It'd be good to get some thoughts from @alyblenkin on this one.
Ah yes, it should go away if you press back to return to the main menu. You wouldn't want to see the snackbar after you've take an action, it should disappear.
I didn't think about this - good point! If the device is connected to the network required for auto-send, we show "It looks like you're offline..." If they have a cellular connection, but no wifi, perhaps we could show "It looks like you don't have Wi-Fi. Your form will send when you are online" or "connected"? I guess they technically have a cellular connection. "It looks like you don't have Wi-Fi. Your form will be sent when you connect" What do you think? |
Now I wonder if it make sense to check internet connection and display it in the snackbar at all...
Taking into account all of that maybe the snackbar should always display |
EDIT: I wrote this before I saw @grzesiek2010's reply (was offline), so some of the comments are redundant. Sorry!
Do you mean if they're connected, but their connection isn't working (like bad signal for instance)? We won't be able to detect that scenario dependably. The only times the phone really knows it's "offline" it can't find a cellular or Wi-Fi network at all. We could try and determine whether we have a "good" connection or not (pinging a known dependable server for instance), but I think that's further than we want to go here (and it's not trivial to do if we're needing to determine the quality of a specific connection type).
I think this approach makes sense. I guess the different scenarios when auto send is enabled we need to cover could look like this:
As I'm writing this I'm realizing there's further complexity here: in situation 2 or 3, I don't think we'll ever auto send the form if the connection type we need is disconnected, but the other is working. This is because (as far as I can tell) we currently wait for any connection to try and auto send, but will then determine whether we have the correct connection type or not and (again, as far as I can see), won't reschedule trying to send again if we don't. This would mean in a case where we have a cellular connection, but no wifi and auto send is "wifi only", we'd actually never auto send the form, so we can't say "Your form will be sent when you connect". @grzesiek2010 could you investigate this to see if I'm correct about those scenarios or not? I think we might want to either rethink what we say on this snackbar, or try and improve the auto send behaviour before adding it (which I'd prefer). |
In such cases, the But again the more I think about it the more I'm convinced that if the button is |
Yeah I agree with this. I think we should go with this for the moment (not have a different message for "offline"), and then look at revising auto send to be less brittle in the future. I think there's a lot we could improve around retrying to send when we have the correct connection that would allow us to more accurately say to the user "Your form will be sent later". Does that sound ok @alyblenkin? If so, let's remove that offline message and hide the snackbar when an action is clicked as discussed here. |
Agreed just giving feedback that a send attempt is ongoing and giving a way to immediately view the form that was just completed is already very helpful. Seems reasonable to focus on that for this release. Have we also considered showing a snackbar when an autosend attempt completes? Currently we show a notification but my experience is that those are really easy to miss and they feel less tied to current actions. I think we may have discussed switching to or adding snackbars at some point. For example:
|
I spoke to @seadowg about the successfully sent message a while back, and we were worried that they could appear out of context or mid-flow when they are filling out another form. I documented some of that conversation here - @seadowg suggested we could maybe leverage system notifications down the road.
What about - "Unable to send. Your form is "Ready to Send" later. View" |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Just a few things inline, but nothing major.
Before merging, it would be good to rebase this on top of master
so that it gets the latest fixes for our instrumentation tests as this is red on Test Lab right now.
collect_app/src/main/java/org/odk/collect/android/activities/InstanceChooserList.java
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/upload/InstanceServerUploader.java
Show resolved
Hide resolved
..._app/src/androidTest/java/org/odk/collect/android/feature/formentry/FormSavedSnackbarTest.kt
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/mainmenu/MainMenuViewModel.kt
Outdated
Show resolved
Hide resolved
9a7f666
to
0dd9e00
Compare
I'm going to mark this "needs testing", as we're only discussing tests now. |
7149347
to
cd6df68
Compare
cd6df68
to
632481b
Compare
I'm not sure what should be an expected result in some scenarios:
In this case the user is moved to the main menu and sees "Your form was saved as a draft"
|
The user is moved to the view of a form with only an "Exit" option which seems ok but on the other hand moving backwards is supposed to prevent edits(?) so the word Edit in snackbar might be confusing? |
Similar scenario to the previous one
The user is moved to the view of a form with only an "Exit" option |
Not perfect but also not something terrible to me. It happens because the snackbar is created whenever after quitting a form its URI is returned. It turns out that when we close a form discarding changes it is returned. I'm not sure if it's the expected behavior but we probably shouldn't touch it so fi we want to improve the current behavior we need to add something to the returned intent when the form is closed to distinguish those cases.
No, the snackbar should be displayed only after closing a form for that particular form.
Here the action name should be
As above the action name should be I also wonder if it makes sense to have that @seadowg what do you think about all those cases? Do you agree with what I said? |
One more question about expected
The users sees a snackbar with "View" option. I don't know if hiding Ready to send and Sent option from the main menu means that the user is not supposed to view forms that are being sent/ were sent? |
Hmm If viewing sent forms was only in the list of sent forms (not in the list of forms to send too) I would say that the View option in the snackbar should not be available but now I don't know... I would file a separate issue for this and discuss it later. |
For 1, I think we should fix this now by not showing a snackbar when exiting a form via "Discard changes/form". In terms of implementation, I think it should be fairly feasible to just not set a result (or set a Totally agree that for cases 3 and 4 we should just change the action text from "Edit" to "View". It might be that there's a helper we can pull out (probably from the logic in I also agree that 5 is something we should discuss as part of another issue - we might not make any changes there. |
1bc0954
to
76ec768
Compare
You mean removing returning URI in that case like in 76ec768 |
@grzesiek2010 Yes that makes sense to me. It's a change in behaviour that will affect external apps (calling into Collect), so we'll need to announce the change explicitly, but that's ok. |
@dbemke already tested this PR Tested With Success! Verified on Device with Android: 10, 12, 13 Verified Cases:
|
Closes #5424
What has been done to verify that this works as intended?
I've tested the changes manually and added new automated tests.
Why is this the best possible solution? Were any other approaches considered?
The most important thing here is how I handled passing an URI of a saved form to the main menu in order to display a snackbar with appropriate data. Initially I did it using shared prefs 4e515a5 but then decided to take advantage of the fact that when a form is saved the URI is passed using activity result. This required some changes 0bfc7ac in the way we start the list of forms and the list of drafts to handle the result but I think this solution is more elegant and consistent.
Another thing of note is that I removed marking forms as failed before sending them see 227adcd
This was implemented to avoid editing forms that should be sent but now we do not allow editing finalized forms at all so it is redundant.
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?
Please review the acceptance criteria in the issue and test saving forms according to them. When a snackbar is being created we assume that a form is saved as draft or finalized (and should be sent or not) but it's not sent yet. It would be good to check if it is possible for example using fast devices and small forms that the form is sent (or failed to send) before the process of creating a snackbar is finished because in such a case it won't be displayed at all.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.