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

IQSS 10390: Multipid UI Fix #10391

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Mar 19, 2024

What this PR does / why we need it: Fixes the issue by adding logic to determine what option will be inherited by a collection - the global default or the value inherited from a parent collection. The PR also addresses the issue with the GET /api/datasets/id/pidGenerator call sometime returning "default" instead of the PidProvider that will be used (as documented in the Guides).

Which issue(s) this PR closes:

Special notes for your reviewer: Suggesting we put this in 6.2 to avoid a bug going out.

Suggestions on how to test this: Have more than one pid provider configured, create a subcollection and verify that the menu of options on the Dataverse/Edit/GeneralInfo pane shows the options, including one that is either the (default) - if no parent collection has a non-default selection - or (inherited...) option - when a parent/grandparent has an explicit PIDProvider setting.
Can also call the API call above and verify that it returns a PidProvider id in all cases.

Does this PR introduce a user interface change? If mockups are available, please link/include them here: see screenshot in inssue.

Is there a release notes update needed for this change?: no - fixes a bug that hasn't yet been released.

Additional documentation:

@qqmyers qqmyers added this to the 6.2 milestone Mar 19, 2024
@coveralls
Copy link

coveralls commented Mar 19, 2024

Coverage Status

coverage: 20.738% (+0.2%) from 20.57%
when pulling b196f54 on GlobalDataverseCommunityConsortium:multipidUI
into 4f46d15 on IQSS:develop.

@qqmyers qqmyers added the Size: 3 A percentage of a sprint. 2.1 hours. label Mar 20, 2024
@sekmiller sekmiller assigned sekmiller and unassigned sekmiller Mar 26, 2024
PidProvider pidProvider = dataset.getEffectivePidGenerator();
if(pidProvider == null) {
//This is basically a config error, e.g. if a valid pid provider was removed after this dataset used it
return error(Response.Status.NOT_FOUND, "No PID Generator found for the give id");
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick here - should be "given" and maybe put it in the bundle. Also should we add a test to PidUtilTest?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

Just an error label update, and maybe an additional test - see comment.

Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

Thanks for the updates

@pdurbin pdurbin self-assigned this Mar 27, 2024
@pdurbin
Copy link
Member

pdurbin commented Mar 27, 2024

Works fine but I didn't like how (inherited from enclosing Dataverse) and (Default) are squished...

Screenshot 2024-03-27 at 11 34 37 AM Screenshot 2024-03-27 at 11 36 16 AM

... so I pushed a fix to add spaces in b196f54

Here's the config I tested with:

    -Ddataverse.pid.providers=fake,foo
    -Ddataverse.pid.default-provider=fake
    -Ddataverse.pid.fake.type=FAKE
    -Ddataverse.pid.fake.label=FakeDOIProvider
    -Ddataverse.pid.fake.authority=10.5072
    -Ddataverse.pid.fake.shoulder=FK2/
    -Ddataverse.pid.foo.type=FAKE
    -Ddataverse.pid.foo.label=Foo
    -Ddataverse.pid.foo.authority=10.1234

Tests are passing. Will merge.

@pdurbin pdurbin merged commit 8915dad into IQSS:develop Mar 27, 2024
3 of 4 checks passed
@pdurbin pdurbin removed their assignment Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MultiPid UI selector incorrect for sub-collections
4 participants