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

Allow editing finalized forms saved before October 1st 2023 UTC #5670

Merged
merged 4 commits into from
Jul 13, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Jul 11, 2023

Closes #5669

What has been done to verify that this works as intended?

I've added automated tests and tested the changes manually.

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

There is nothing important 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?

Generally opening forms for edit/view only is at risk. To test the case described in the issue you need to change dates manually in the app (after October 1st 2023) but please also make sure here is no regression in other related cases.

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:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • 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 base branch from master to v2023.2.x July 11, 2023 20:45
@grzesiek2010 grzesiek2010 changed the base branch from v2023.2.x to master July 11, 2023 20:45
@grzesiek2010
Copy link
Member Author

One thing we haven't discussed is the snackbar. If we finalize a form it contains the view action but the form can be edited. I think we should update it too right? @lognaturel @alyblenkin

@alyblenkin
Copy link
Collaborator

@grzesiek2010 - good point! I'm tempted to leave it as view instead of changing it to edit so they don't expect that to be the new behaviour. I know that's confusing because they can edit it, but this is only a temporary solution to help folks transition to the new workflow.

@grzesiek2010 grzesiek2010 marked this pull request as ready for review July 12, 2023 08:14
@@ -181,6 +182,9 @@ public void viewForm_doesNotRecordAudio() {
.startBlankForm("One Question")
.fillOutAndFinalize(new FormEntryPage.QuestionAndAnswer("what is your age", "17"))
.clickSendFinalizedForm(1)
.clickSelectAll()
Copy link
Member

Choose a reason for hiding this comment

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

Why is the form being sent here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because only if the form can't be edited recording should not be started again. Now if we just finalize, the forms are editable.

Copy link
Member

@seadowg seadowg Jul 12, 2023

Choose a reason for hiding this comment

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

Ah right. I guess this is linked to the below discussion then. Feels like it'd be good to lock in one version for the behaviour in tests.

.assertTextDoesNotExist(R.string.jump_to_beginning)
.assertTextDoesNotExist(R.string.jump_to_end)
.assertText(R.string.exit)
.assertText(R.string.jump_to_beginning)
Copy link
Member

Choose a reason for hiding this comment

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

This test (and the modified one in SendFinalizedFormTest will start failing after October 1st right?

Maybe we should flag this feature out in instrumentation tests or lock the clock to specific time (probably via Dagger and then passing it down through InstancesRepositoryProvider).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about this but:

  1. I wanted to quickly bring a fix for the issue.
  2. I think it's better to have tests for the current state even if it's weird and then in October we can fix it instead of mocking the clock and having tests that verify what we expect to have in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I don't have strong feelings about it so I added separate tests fo the grace period and after.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I like that even more!

@seadowg seadowg added needs testing high priority Should be looked at before other PRs/issues labels Jul 12, 2023
@seadowg
Copy link
Member

seadowg commented Jul 12, 2023

@grzesiek2010 looks like there's an "optimise imports" needed.

@srujner
Copy link

srujner commented Jul 13, 2023

Tested with Success

Verified on device with Android 12,13

Verified cases:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't edit a form finalized in a previous version of Collect
4 participants