Fixes: error handling on New Form when underlying file is changed.#1491
Fixes: error handling on New Form when underlying file is changed.#1491sadiqkhoja wants to merge 1 commit intogetodk:masterfrom
Conversation
| await expect(page.locator('.modal-warnings')).not.toBeVisible(); | ||
| } finally { | ||
| // Clean up temp directory | ||
| // fs.rmSync(tempDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
Should this be commented back in? Or if it's not needed, could the whole try/finally be removed?
| } | ||
|
|
||
| // Assumption: If file is changed/deleted on the disk then it won't be readable | ||
| if (ignoreWarnings) { |
There was a problem hiding this comment.
I don't totally understand yet why this if is needed. I feel like it'd be helpful in all cases to read the file and check for errors.
Case 1: File modification between file selection and initial upload
- Open the modal.
- Select a file.
- You realize you forgot to save the file — now you do so.
- Click the upload button.
- Error due to file modification
Case 2: File modification after warnings receipt
- Open the modal.
- Select a file.
- Click the upload button. A warning is returned.
- Fix the file and save it.
- Click the upload button.
- Error
Proposal
- Always read the file and check for error before sending the request.
- Don't reset
this.fileafter a request error. In that case, we know the error isn't due to file modification.
I'm guessing I don't understand some part of this, so I just wanted to share my current thinking. Does this approach not work because the file can only be read once?
There was a problem hiding this comment.
ah; Case 1 is intermittent failure for me (at least on Firefox). Last time when I was working on this, browser was able to read the changed file before upload button click.
I like your proposal let me try that.
| ]; | ||
| const devServer = { | ||
| port: 8989, | ||
| host: true, |
There was a problem hiding this comment.
Could you say more about why this change is needed?
There was a problem hiding this comment.
it is needed to run e2e tests locally otherwise playwright is unable to resolve the host.
but this change shouldn't be part of this PR.
Community member has also faced related issue: #1274
| this.file = null; | ||
| this.warnings = null; |
There was a problem hiding this comment.
This pattern of clearing both this.file and this.warnings also appears in the state watcher. I wonder if it'd make things clearer to add a method to do so (named maybe clear or reset). Just a thought / no strong feelings.
|
|
||
| deleteForm = async (xmlFormId) => { | ||
| const request = await this.#getRequest(); | ||
| const response = await request.delete(`/v1/projects/${projectId}/forms/${xmlFormId}`); |
There was a problem hiding this comment.
Is it worth encoding xmlFormId in the URL? Or is that never a problem in e2e tests?
| await backendClient.dispose(); | ||
| }); | ||
|
|
||
| const login = async (page) => { |
There was a problem hiding this comment.
Since this function is used in other e2e tests, what do you think about making it reusable somehow? Like creating a util file?
| }; | ||
|
|
||
| test.describe('Form Upload', () => { | ||
| test('clears file input when server returns an error', async ({ page }) => { |
There was a problem hiding this comment.
I understand why the next e2e test is useful: it's hard to mock file modification in component tests. But this one I don't feel as sure about. I feel like this one could be tested in component tests easily enough (or maybe already is?). We frequently mock server errors in component tests.
There was a problem hiding this comment.
getodk/central#1291 (comment) makes the point that this issue comes up in other cases as well:
this issue affects file uploads generally, not just forms. I also encounter the issue on the form attachments page
Given that, I feel like this change doesn't fully close out #1291.
Maybe one option is to file a follow-up issue about form attachments. This could probably come up for entity upload as well. Though it could be that those are rare cases. Certainly form upload seems like the most important case. 👍
2 participants
Closes getodk/central#1291 and getodk/central#1558
What has been done to verify that this works as intended?
Added e2e tests and manual verification
Why is this the best possible solution? Were any other approaches considered?
I'd prefer to use File System API and upload the latest file but it is not fully supported in Firefox. With File System API, iterating on Form Design would be really easy. In the absence of that, I went with the following approach:
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?
None
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
No
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes