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

Handle not existing forms and form files during bulk finalize #5835

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Nov 14, 2023

Closes #5833

Why is this the best possible solution? Were any other approaches considered?

Nothing to discuss here.

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?

This should only fix the crashes mentioned in the issue and have no side effects. I can't think of any risk here.

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:

  • added or modified tests for any new or changed behavior
  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 changed the title Collect 5833 Handle not existing forms and form files during bulk finalize Nov 14, 2023
@lognaturel
Copy link
Member

It looks like this will show the finalization attempt as failed but won't give any context as to why, is that right? The user won't have any hints about what happened or how they could fix it? Is that better than just crashing?

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Nov 15, 2023

It looks like this will show the finalization attempt as failed but won't give any context as to why, is that right?

Correct.

the user won't have any hints about what happened or how they could fix it? Is that better than just crashing?

To me of course it's better:

  1. The message displayed after reopening the app wouldn't be helpful at all:
    image

  2. If you have many filled forms and only one or some of them can't be finalized because of this issue then still the rest of the forms will be finalized. If you have a crash the process is stopped.

  3. A user can try to open non-finalized forms and then see this, which is helpful:
    image

@lognaturel
Copy link
Member

Great, I didn’t think through what they’d see if they try to open the form. Makes sense!

@dbemke
Copy link

dbemke commented Nov 20, 2023

On Android 10 and 13 after reproducing steps from the issue there is a crash.

Save any form as draft.
Delete the form file manually from the project directory (the one from the forms subdirectory)
Open the list of drafts and try to bulk finalize the saved forms

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Nov 21, 2023

On Android 10 and 13 after reproducing steps from the issue there is a crash.

What exactly are you removing? Only the XML file or something else? I've just tried and everything seems to be fine.
Does it only occur on Android 10 and 13 or you have just tried those two devices?

@srujner
Copy link

srujner commented Nov 21, 2023

I removed just the Xml document from the Files -> projects -> Name of the project -> instances -> Name of the form.

I tried only on Android 13 so far

@grzesiek2010
Copy link
Member Author

I removed just the Xml document from the Files -> projects -> Name of the project -> instances -> Name of the form.

That's a completely new case different than those two I've described in the issue. But ok we can handle this one too.

@srujner
Copy link

srujner commented Nov 21, 2023

So what's the one mention in here:
"Delete the form file manually from the project directory (the one from the forms subdirectory)"

Deleting the whole folder?

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Nov 21, 2023

So what's the one mention in here:
"Delete the form file manually from the project directory (the one from the forms subdirectory)"

Deleting the whole folder?

No, the difference is that I said: delete the file from the forms subdirectory and you removed the one from the instances subdirectory. So instead of removing the blank form file, you removed the filled form file.

@dbemke
Copy link

dbemke commented Nov 22, 2023

We reproduced the steps from the issue with resetting blank forms and the saved drafts has "No errors" pill but after trying to bulk finalize there's the snackbar with information that the form contains errors. Is it expected?
Noerrorsdeletedblanks

@dbemke
Copy link

dbemke commented Nov 22, 2023

Is it the expected error message after removing the form subdirectory and trying to open the saved instance of the form?

deletedFormSubdirectory

@grzesiek2010
Copy link
Member Author

We reproduced the steps from the issue with resetting blank forms and the saved drafts has "No errors" pill but after trying to bulk finalize there's the snackbar with information that the form contains errors. Is it expected?

Yes, those are edge cases and not errors in forms themselves so it's ok as it is.

Is it the expected error message after removing the form subdirectory and trying to open the saved instance of the form?

I would say yes. The message could be better maybe but is not that bad if a user reads it, it's possible to figure out the reason.

@srujner
Copy link

srujner commented Nov 22, 2023

Tested with Success!

Verified on device with Android 13

Verified cases:

@dbemke
Copy link

dbemke commented Nov 22, 2023

Tested with Success!

Verified on device with Android 10

@grzesiek2010 grzesiek2010 merged commit a783893 into getodk:v2023.3.x Nov 22, 2023
6 checks passed
seadowg pushed a commit to seadowg/collect that referenced this pull request Nov 30, 2023
Handle not existing forms and form files during bulk finalize
seadowg pushed a commit to seadowg/collect that referenced this pull request Dec 7, 2023
Handle not existing forms and form files during bulk finalize
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The app crashes if a user bulk finalizes drafts that do not have their form definitions
5 participants