BF: avoid clobbering of dict(..., **rec) type and always add a record for the author for new dandisets #2377
Conversation
|
I verified this against my branch, and it works as expected, still yet to adjust the testing though. |
… for the author for new dandiset Closes #2376 @asmacdo reported that Embergoed dandisets have a difference of not having their author automatically added to the metadata. Identified via visual inspection this piece of code where there contributors list would be populated with the entry provided with the funder. This code seems to be used only in create_dandiset so I do not see much of side-effects
|
@yarikoptic ready for review |
|
dandi-cli fails suspicious -- relate to author name... and |
If the superuser is used to create any dandisets (happens in tests) then the superuser is automatically added as a contributor If the superuser does not have a name, the dandiset version will be invalid.
|
Huzzah! Requires PR logic worked, and we are down to 6 failing tests rather than 17! Looking into the remaining failures now. |
|
I've made additional changes to the expected data in the CLI tests in dandi/dandi-cli#1647 so hopefully we are green now! |
|
Required PR logic only affects the master test for DANDI cli-integration, it doesn't affect release, so that failure is expected, but should serve as a warning that we should merge with caution. |
| - name: Install dev dandi | ||
| if: matrix.dandi-version == 'master' | ||
| run: pip install "dandi[test] @ git+https://github.com/dandi/dandi-cli" | ||
| - name: Extract CLI PR reference from description |
There was a problem hiding this comment.
what is all this and why we need it for this fix testing?
There was a problem hiding this comment.
I opened a PR against dandi-cli, this lets us test against cli PRs
There was a problem hiding this comment.
I can open another PR for this, but since I wanted to use it I figured it would be good to at least test it on this one.
|
|
||
| # Set first_name and last_name from environment variables if provided | ||
| first_name = os.environ.get('DJANGO_SUPERUSER_FIRST_NAME') | ||
| last_name = os.environ.get('DJANGO_SUPERUSER_LAST_NAME') |
There was a problem hiding this comment.
see if could pass as CLI options for this management command instead of relying on internally defined env vars here. Then adjust dandi-cli fixture to provide some name.
Requires_CLI_PR: Add superuser name for creation dandi-cli#1647
Closes Embargoed Dandisets missing ContactPerson #2376
@asmacdo reported that Embergoed dandisets have a difference of not having their author automatically added to the metadata. Identified via visual inspection this piece of code where there contributors list would be populated with the entry provided with the funder. This code seems to be used only in
create_dandisetso I do not see much of side-effects./end original
The CLI integration tests failed with the original bugfix due to the fixture dandiset behavior. When the
createsuperusermanagement command is called, the user is created without a first_name or last_name. This can be seen in a development environment if./manage.py create_dev_dandiset --owner <superuseremail>is called before using the GUI to set a name for the user-- the resulting dandiset is invalid. After logging in to the GUI, the user gets a name, and subsequent calls tocreate_dev_dandisetusually (but not always) produce valid dandisets.In the CLI test fixtures, this problem was overridden by providing a complete list of contributors, excluding the superuser. However, with this bugfix, the original contributors are no longer clobbered, which left many of the dandisets with a contributor that had ", " as their name, which broke validation.
To get the tests to pass, the
createsuperusernow acceptsDJANGO_SUPERUSER_FIRST_NAMEandDJANGO_SUPERUSER_LAST_NAME, and dandi-cli test fixture has been modified to use them, so fixture dandisets are now valid. I had to alter the expected values of the metadata to include the superuser in the cli tests as well.There did not exist any mechanism to use a dandi-cli pull request in the tests, so I've added functionality to check the dandi-archive PR body for a required CLI-PR, and if present, checkout that branch and run the tests. I hope that this functionality will be useful for other cross-repo breaking changes, and can be extended for dandi-schema as well.
@yarikoptic this is ready for review but I am going to leave as a draft, because what should have just been a minor bugfix required such invasive changes, and the tests against
releasewill be broken. IMO even if we choose to not keep this bugfix or thecreatesuperuserchanges, I hope that "require dandi-cli pr" functionality will still be useful, and can be easily broken into a separate PR.@asmacdo would continue to