Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions e2e-tests/backend-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ export default class BackendClient {
expect(response).toBeOK();
};

deleteForm = async (xmlFormId) => {
const request = await this.#getRequest();
const response = await request.delete(`/v1/projects/${projectId}/forms/${xmlFormId}`);
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth encoding xmlFormId in the URL? Or is that never a problem in e2e tests?

expect(response).toBeOK();
};

createFormAndChildren = async () => {
const form = await this.createForm();
const submission = await this.createSubmission(form.xmlFormId);
Expand Down
23 changes: 23 additions & 0 deletions e2e-tests/data/form-without-meta.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms"
xmlns:h="http://www.w3.org/1999/xhtml"
xmlns:jr="http://openrosa.org/javarosa"
xmlns:odk="http://www.opendatakit.org/xforms"
xmlns:orx="http://openrosa.org/xforms">
<h:head>
<h:title>Missing meta element</h:title>
<model>
<instance>
<data id="missing-meta">
<firstname/>
</data>
</instance>
<bind nodeset="/data/firstname"/>
</model>
</h:head>
<h:body>
<input ref="/data/firstname">
<label>First Name</label>
</input>
</h:body>
</h:html>
117 changes: 117 additions & 0 deletions e2e-tests/tests/form-upload.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { test, expect } from '@playwright/test';
import fs from 'fs';
import os from 'os';
import path from 'path';
import BackendClient from '../backend-client';

const appUrl = process.env.ODK_URL;
const user = process.env.ODK_USER;
const password = process.env.ODK_PASSWORD;
const projectId = process.env.PROJECT_ID;

let publishedForm;

test.beforeAll(async ({ playwright }, testInfo) => {
const backendClient = new BackendClient(playwright, `${testInfo.project.name}_form_upload`);
await backendClient.alwaysHideModal();
publishedForm = await backendClient.createForm();
await backendClient.dispose();
});

const login = async (page) => {
Copy link
Member

Choose a reason for hiding this comment

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

Since this function is used in other e2e tests, what do you think about making it reusable somehow? Like creating a util file?

await page.goto(appUrl);
await expect(page.getByRole('heading', { name: 'Log in' })).toBeVisible();

await page.getByPlaceholder('email address').fill(user);
await page.getByPlaceholder('password').fill(password);

await page.getByRole('button', { name: 'Log in' }).click();

await page.waitForURL(appUrl);
};

test.describe('Form Upload', () => {
test('clears file input when server returns an error', async ({ page }) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

await login(page);

// Navigate to create form page
await page.goto(`${appUrl}/#/projects/${projectId}`);
await page.getByRole('button', { name: 'New' }).click();

// Verify modal is open
await expect(page.locator('#form-new')).toBeVisible();

const formXml = path.join(__dirname, '../data/form-without-meta.xml');

await page.locator('#form-new input[type="file"]').setInputFiles(formXml);

// Verify filename is displayed
await expect(page.locator('#form-new-filename')).toContainText('form-without-meta');

// Click upload
await page.getByRole('button', { name: 'Upload' }).click();

// Wait for error alert to appear
await expect(page.locator('#form-new .red-alert')).toContainText("The form does not contain a 'meta' group");

// Verify file input is cleared (filename should not be visible)
await expect(page.locator('#form-new-filename')).not.toBeVisible();
});

test('shows error when file is modified before clicking upload anyway', async ({ page, playwright }, testInfo) => {
// Delete the form to put it in trash
const backendClient = new BackendClient(playwright, `${testInfo.project.name}_form_upload`);
await backendClient.deleteForm(publishedForm.xmlFormId);
await backendClient.dispose();

// Create a temp file with the same formId as the deleted form
const formTemplate = fs.readFileSync(
path.join(__dirname, '../data/form.template.xml'),
'utf8'
);
const formXml = formTemplate.replaceAll('{{ formId }}', publishedForm.xmlFormId);
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'form-upload-test-'));
const tempFilePath = path.join(tempDir, `${publishedForm.xmlFormId}.xml`);
fs.writeFileSync(tempFilePath, formXml);

try {
await login(page);

// Navigate to create form page
await page.goto(`${appUrl}/#/projects/${projectId}`);
await page.getByRole('button', { name: 'New' }).click();

// Verify modal is open
await expect(page.locator('#form-new')).toBeVisible();

// Upload the temp file
await page.locator('#form-new input[type="file"]').setInputFiles(tempFilePath);

// Verify filename is displayed
await expect(page.locator('#form-new-filename')).toContainText(publishedForm.xmlFormId);

// Click upload
await page.getByRole('button', { name: 'Upload' }).click();

// Expect a warning that form with the same ID exists in the trash
await expect(page.locator('.modal-warnings')).toBeVisible();
await expect(page.locator('.modal-warnings')).toContainText('Trash');

// Modify the temp file
fs.writeFileSync(tempFilePath, formXml.replaceAll(publishedForm.xmlFormId, `${publishedForm.xmlFormId}_new`));

// Click "Upload anyway"
await page.getByRole('button', { name: 'Upload anyway' }).click();

// Expect error that file has been modified
await expect(page.locator('#form-new .red-alert')).toContainText('could not be read');

// Verify file input and warnings are cleared
await expect(page.locator('#form-new-filename')).not.toBeVisible();
await expect(page.locator('.modal-warnings')).not.toBeVisible();
} finally {
// Clean up temp directory
// fs.rmSync(tempDir, { recursive: true, force: true });
Copy link
Member

Choose a reason for hiding this comment

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

Should this be commented back in? Or if it's not needed, could the whole try/finally be removed?

}
});
});
23 changes: 21 additions & 2 deletions src/components/form/new.vue
Copy link
Member

Choose a reason for hiding this comment

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

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. 👍

Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,21 @@ export default {
return;
}

// Assumption: If file is changed/deleted on the disk then it won't be readable
if (ignoreWarnings) {
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. Open the modal.
  2. Select a file.
  3. You realize you forgot to save the file — now you do so.
  4. Click the upload button.
    • Error due to file modification

Case 2: File modification after warnings receipt

  1. Open the modal.
  2. Select a file.
  3. Click the upload button. A warning is returned.
  4. Fix the file and save it.
  5. Click the upload button.
    • Error

Proposal

  • Always read the file and check for error before sending the request.
  • Don't reset this.file after 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 reader = new FileReader();
reader.onload = () => this.performUpload(ignoreWarnings);
reader.onerror = () => {
this.redAlert.show(this.$t('alert.fileNotReadable'));
this.file = null;
this.warnings = null;
Comment on lines +209 to +210
Copy link
Member

Choose a reason for hiding this comment

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

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.

};
reader.readAsArrayBuffer(this.file);
} else {
this.performUpload(ignoreWarnings);
}
},
performUpload(ignoreWarnings) {
const query = ignoreWarnings ? { ignoreWarnings } : null;
const headers = { 'Content-Type': this.contentType };
if (this.contentType !== 'application/xml') {
Expand Down Expand Up @@ -242,7 +257,10 @@ export default {
}
})
.catch(() => {
if (this.$route === initialRoute) this.warnings = null;
if (this.$route === initialRoute) {
this.warnings = null;
this.file = null;
}
});
},
removeLearnMore(value) {
Expand Down Expand Up @@ -314,7 +332,8 @@ export default {
"uploadAnyway": "Upload anyway"
},
"alert": {
"fileRequired": "Please choose a file."
"fileRequired": "Please choose a file.",
"fileNotReadable": "The file could not be read. It may have been modified or deleted. Please choose the file again."
},
"problem": {
"400_8": "The Form definition you have uploaded does not appear to be for this Form. It has the wrong formId (expected “{expected}”, got “{actual}”).",
Expand Down
2 changes: 1 addition & 1 deletion test/components/form/new.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ describe('FormNew', () => {
modal.should.alert('danger');
modal.get('.modal-warnings').should.be.hidden();
})
.request(modal => modal.get('#form-new-upload-button').trigger('click'))
.request(upload)
.respondWithProblem(xlsFormWarning)
.afterResponse(modal => {
modal.should.not.alert();
Expand Down
3 changes: 3 additions & 0 deletions transifex/strings_en.json
Original file line number Diff line number Diff line change
Expand Up @@ -3431,6 +3431,9 @@
"alert": {
"fileRequired": {
"string": "Please choose a file."
},
"fileNotReadable": {
"string": "The file could not be read. It may have been modified or deleted. Please choose the file again."
}
},
"problem": {
Expand Down
1 change: 1 addition & 0 deletions vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const proxyPaths = [
];
const devServer = {
port: 8989,
host: true,
Copy link
Member

Choose a reason for hiding this comment

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

Could you say more about why this change is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

proxy: Object.fromEntries(proxyPaths.map(path => [path, 'http://localhost:8686'])),
// Because we proxy to nginx, which itself proxies to Backend and other
// things, the dev server doesn't need to allow CORS. CORS is already limited
Expand Down