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

Fix Embargo #837

Merged
merged 21 commits into from
Jun 11, 2024
Merged

Fix Embargo #837

merged 21 commits into from
Jun 11, 2024

Conversation

garrettmflynn
Copy link
Member

This PR is an attempt to fix all issues relating to embargoed Dandisets.

I've been able to isolate the embargoed Dandiset issue to the fact that requesting dandisets/?user=me does not appear to return Dandisets that are embargoed. I'll start scouring the docs for an alternative request.

Once this is complete, we can move on to #814.

@garrettmflynn garrettmflynn self-assigned this Jun 5, 2024
@CodyCBakerPhD CodyCBakerPhD linked an issue Jun 5, 2024 that may be closed by this pull request
@garrettmflynn
Copy link
Member Author

A simple fix. Turns out this request turns everything except embargoed Dandisets by default. Need to add another search query (e.g. dandisets/?user=me&embargoed=true) to the end of it.

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jun 5, 2024

@CodyCBakerPhD Aside from the NIH Award Number stuff, can you confirm this is behaving as expected? You will need to update your JavaScript dependencies.

@CodyCBakerPhD
Copy link
Collaborator

When I try to create one one on staging I get this error

dandi.es.js:199 Uncaught (in promise) Error: API is not authorized. Please provide a valid token.
    at K.authorize (dandi.es.js:199:15)
    at async K.init (dandi.es.js:182:7)
    at async Button.onClick [as __onClick] (UploadsPage.js:122:17)

But it's definitely the right credentials for staging

@CodyCBakerPhD
Copy link
Collaborator

I also can't see any of my embargoed Dandisets on the loose selector when my API keys are set

@CodyCBakerPhD
Copy link
Collaborator

Also this spams on the console (but not in dev tools view)

[8404:0605/171144.344:ERROR:CONSOLE(6)] "console.assert", source: devtools://devtools/bundled/panels/console/console.js (6)

@garrettmflynn
Copy link
Member Author

Sorry! Try again

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review June 10, 2024 14:10
@CodyCBakerPhD
Copy link
Collaborator

OK I can now see them in the selector

One thing, not sure if you want to deal with it in this PR or a follow-up : the NIH grant number validation did not occur until I hit 'Submit' (I clicked off it and waited to see if that would trigger but it did not) and then it popped up very briefly but submission of the modal finished (would have expected the modal to stay open if there was an error) before I could fix it

However, the dandiset did get created and seems fine. Maybe it would have problems at some level without the right kind of grant ID format, but the basic feature did work

@garrettmflynn
Copy link
Member Author

Let's handle the NIH grant number stuff before merging this PR, as this will enable embargo again. I'll start this in a linked follow-up.

@garrettmflynn
Copy link
Member Author

Alright invalid NIH grant numbers are properly blocked now! I'm going to skip the auto-parsing of the non-delimited string copied off NIH Reporter because DANDI itself does not accept this format directly.

@CodyCBakerPhD
Copy link
Collaborator

Cool

CI tests failing now - you wanted to work on NIH number formatting on a separate branch/PR that gets merged into this one before merging either to main?

@garrettmflynn garrettmflynn linked an issue Jun 10, 2024 that may be closed by this pull request
@garrettmflynn
Copy link
Member Author

Blocking Dandiset creation when the user provides an invalid NIH grant number has been implemented here. I'm actually going to skip the number formatting since DANDI does not allow this. Would it be correct here to assume we want to maintain parity between ourselves and DANDI?

@CodyCBakerPhD
Copy link
Collaborator

I think it's probably fine at this point, the other idea was a 'nicety'

As per meeting; we can merge this whenever you're ready but I won't cut a new release until after user days

@garrettmflynn
Copy link
Member Author

Sounds good to me. Will need your review here

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge June 11, 2024 16:18
auto-merge was automatically disabled June 11, 2024 16:18

Pull request was closed

@CodyCBakerPhD CodyCBakerPhD reopened this Jun 11, 2024
@CodyCBakerPhD CodyCBakerPhD enabled auto-merge June 11, 2024 16:18
@CodyCBakerPhD CodyCBakerPhD merged commit 04b34a6 into main Jun 11, 2024
37 of 40 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the fix-embargo branch June 11, 2024 16:47
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.

Improve NIH Award Number Handling Cannot see my embargoed dandisets
2 participants