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

Edf fields #215

Merged
merged 20 commits into from
Nov 24, 2022
Merged

Edf fields #215

merged 20 commits into from
Nov 24, 2022

Conversation

dandavies99
Copy link
Contributor

@dandavies99 dandavies99 commented Nov 21, 2022

This adds two new optional fields to the ExperimentDataFile, one for "settings file" and one for "binary file". These are just FileFields as opposed to UploadedFile objects because they never need to be parsed or have any of the other properties of data files.

New validators have been created to check the files meet certain criteria. These may need to be reviewed in future as different types of file (with different allowed extensions) come to light.

closes #179

Copy link
Contributor

@CWestICL CWestICL left a comment

Choose a reason for hiding this comment

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

When uploading binary files or settings files to existing Experiment Data file object, an error message was thrown saying the uploaded file was invalid. However, going back to the experiment's page showed the files had actually uploaded and could be downloaded.

When uploading binary files or settings files for a new Experiment Data file object, the error was thrown again but recognised the files were invalid and did not create the new Data object.

@AdrianDAlessandro
Copy link
Collaborator

I suspect this behaviour is again in the logic for when to save/not save objects in the NewDataFileView. Appears to be need some customisation to allow for editing existing files (is this maybe related to #181 and not caused by this PR?)

@dandavies99
Copy link
Contributor Author

dandavies99 commented Nov 23, 2022

Yeah, in general the logic for UpdateDataFileView needs checking since all these changes. We chose to not allow users to use that method to change the data file that had been uploaded, which is reflected in the __init__ method of NewExperimentDataFileForm. I can't remember why this was, maybe we don't need to do that now.

So the form is valid, which means the ExperimentDataFile object gets saved including the new settings and/or binary file(s) (self.object.save()) but the formset is not valid because... it's not there! The form doesn't show it, just a message saying you can't change the raw data file.

So either, in the view we can remove the check for the formset being valid, or we can choose to allow users to change the file they've uploaded.

@dandavies99
Copy link
Contributor Author

For simplicity, I've opted for option a - removing the unused formset from the UpdateView - in the latest commit, which looks like has done the trick.

@dandavies99 dandavies99 requested a review from CWestICL November 23, 2022 09:17
Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

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

Everything seems good now!

tests/battDB/test_views.py Outdated Show resolved Hide resolved
tests/battDB/test_views.py Outdated Show resolved Hide resolved
dandavies99 and others added 3 commits November 23, 2022 16:47
@dandavies99 dandavies99 merged commit 196e627 into develop Nov 24, 2022
@CWestICL
Copy link
Contributor

Just been testing and noticed that when uploading an invalid settings or binary file when editing existing experiment data, the files are still uploaded despite the error message flagging them. Isn't an issue for new experiment data as it successfully blocks the upload there.

Changes have been merged already so I guess this should be put into a new issue?

@AdrianDAlessandro
Copy link
Collaborator

Well spotted @CWestICL! I thought Dan had fixed that with b7e093e but apparently I didn't check it properly...

@dandavies99
Copy link
Contributor Author

That's strange. Maybe because in the UpdateDataFileView the self.object.save() happens before the file validation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional file fields on ExperimentDataFile
3 participants