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

remove NONE as a license option for SWORD, show available licenses #8551 #8558

Merged
merged 10 commits into from
Apr 5, 2022

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Mar 30, 2022

What this PR does / why we need it:

The example XML in the SWORD docs doesn't work due to licenses changes in 5.10 and PR #7920.

Which issue(s) this PR closes:

Special notes for your reviewer:

I added some minimal testing of interactions between license and rights. We could add more.

I'm not 100% sure of all the assertions in the SWORD page of the API Guide. We could add more tests and/or edit the guide as needed.

Generally speaking, I'm thinking we should drop NONE as an option, even if it's an incompatible change. If we want to keep NONE, what should it map to? Custom Dataset Terms? But they can't be blank, right?

Suggestions on how to test this:

  • Try passing NONE. You should now see a list of possible licenses.
  • Try passing a valid license.
  • Test all the assertions in the SWORD section of the API Guide about license, rights, etc.

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

No.

Is there a release notes update needed for this change?:

Maybe?

Additional documentation:

None.

Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

Looks OK - I didn't run it.

If I understand, the only main code change is to add the list of active licenses in the response.
My comment there suggests adding a check to make sure the request License is active, which would be a minor functionality change.

The other changes fix the example, add good tests, update docs. I saw one more CC0 that could be changed but it all makes sense.

@pdurbin pdurbin self-assigned this Mar 31, 2022
@pdurbin pdurbin removed their assignment Mar 31, 2022
@coveralls
Copy link

coveralls commented Mar 31, 2022

Coverage Status

Coverage decreased (-0.001%) to 18.896% when pulling 09e1c60 on 8551-sword-license into e4eb675 on develop.

@pdurbin pdurbin added this to the 5.10.1 milestone Apr 4, 2022
@kcondon kcondon self-assigned this Apr 5, 2022
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.

As of 5.10, SWORD API no longer accepts NONE for dcterms:license
4 participants