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

Mandatory names #224

Merged
merged 12 commits into from
Dec 6, 2022
Merged

Mandatory names #224

merged 12 commits into from
Dec 6, 2022

Conversation

AdrianDAlessandro
Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro commented Nov 29, 2022

This begins the tasks set out in #118 (comment) beginning with the making the name field mandatory and unique for most of the models.

This almost works. It's failing with 'NoneType' object has no attribute 'institution' because the user_owner attribute of the Experiment is not set until after the form validity is checked here https://github.com/ImperialCollegeLondon/Faraday-liionsden/blob/develop/common/views.py#L74

Also, it is currently based from the auto-mass branch.

Edit: I found a way to add the user to the form before checking its validity. I believe this works as intended now and should make all the other forms that use this base form more robust.

It is now based off the develop branch, and should be merged after #220

@dandavies99
Copy link
Contributor

Yeah it looks like we might need to adjust that view. I can't see a way of doing it without user_owner being set before is_valid(). There are some unit tests for battDB.views so hopefully it'll be obvious if a change breaks anything, although we probably should expand the coverage of those tests as a matter of priority.

@AdrianDAlessandro AdrianDAlessandro marked this pull request as ready for review December 2, 2022 16:14
@AdrianDAlessandro AdrianDAlessandro changed the base branch from auto-mass to develop December 2, 2022 16:15
@dandavies99 dandavies99 changed the base branch from develop to auto-mass December 5, 2022 12:43
@dandavies99 dandavies99 changed the base branch from auto-mass to develop December 5, 2022 12:43
Copy link
Contributor

@dandavies99 dandavies99 left a comment

Choose a reason for hiding this comment

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

Looks great! Nice addition to the tests too. Just a couple of small suggestions and a question about having unique=True in the abstract base model.

common/models.py Show resolved Hide resolved
common/views.py Outdated Show resolved Hide resolved
tests/battDB/baker_recipes.py Outdated Show resolved Hide resolved
tests/battDB/test_views.py Show resolved Hide resolved
@dandavies99 dandavies99 merged commit f4a5ac4 into develop Dec 6, 2022
@dandavies99 dandavies99 deleted the mandatory-names branch December 13, 2022 10:49
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.

2 participants