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

8671 license sorting #8697

Merged
merged 47 commits into from
Nov 21, 2022
Merged

8671 license sorting #8697

merged 47 commits into from
Nov 21, 2022

Conversation

ErykKul
Copy link
Collaborator

@ErykKul ErykKul commented May 16, 2022

What this PR does / why we need it:
Right now licenses are sorted only by the ID and this order cannot be changed. With this pull request, admins can sort the licenses with the API calls, where the ordering by ID remains default until changed by manually sorting (order by sortOrder, id). SortOrder is a new column in license that becomes also visible in JSON representation of the license.

Which issue(s) this PR closes:

@coveralls
Copy link

coveralls commented May 20, 2022

Coverage Status

Coverage decreased (-0.008%) to 19.974% when pulling bb1865c on ErykKul:8671_license_sorting into f83e508 on IQSS:develop.

@sekmiller sekmiller self-assigned this May 26, 2022
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.

Looks good generally, but we should probably update the doc and example files to reflect that the add license api requires a sort order

@ErykKul
Copy link
Collaborator Author

ErykKul commented May 31, 2022

I have updated the documentation and the examples. However, I have also corrected the pre-installed licenses list in documentation by removing CC BY as V5.9.0.1__7440-configurable-license-list.sql does not insert it and I could not find any other place where this could happen. I am not sure about this.

@pdurbin
Copy link
Member

pdurbin commented Jun 2, 2022

@ErykKul hi! The CC BY license is added in scripts/api/setup-all.sh. Please see this PR:

Some more thoughts on what the source of truth should be for licenses (JSON used in setup scripts vs. Flyway scripts): #8407 (comment)

@ErykKul
Copy link
Collaborator Author

ErykKul commented Jun 2, 2022

@pdurbin I have reverted the removal of the CC BY references in the documentation.

@sekmiller sekmiller removed their assignment Jul 12, 2022
@pdurbin
Copy link
Member

pdurbin commented Jul 13, 2022

I know this PR is already in QA but I looked anyway and wanted to note a couple things.

  • I'm seeing "Sort order field is mandatory." This should probably be emphasized in the release note snippet. It can be put under a ##Backward Incompatibilities heading. @ErykKul do you want to do this?
  • I'm off campus so I can't see why the Jenkins tests failed. If someone could check why, that would be great.

@qqmyers
Copy link
Member

qqmyers commented Jul 13, 2022

looks like the LicensesIT test is failing due to lack of that sort order value: {\"status\":\"ERROR\",\"message\":\"There should be a sort order value in the request body\"}\n[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.886 s <<< FAILURE! - in edu.harvard.iq.dataverse.api.LicensesIT\n[ERROR] edu.harvard.iq.dataverse.api.LicensesIT.testLicenses

@ErykKul
Copy link
Collaborator Author

ErykKul commented Nov 10, 2022

There was a problem with my flyway script. I have fixed it and got the integration test to run and pass.

@kcondon kcondon self-assigned this Nov 14, 2022
@kcondon
Copy link
Contributor

kcondon commented Nov 15, 2022

This works, waiting for integration tests to run before merging.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Nov 16, 2022

OK. I will try to fix the jenkins build (I cannot see the output, I assume it fails because of the not null column without default value) by dropping the column first and then reverting the drop.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Nov 16, 2022

@kcondon I still had a problem with preparation script as the column generated by eclipse-link was not the same as the one added by the flyway script (making this column nullable was the most difficult thing that I did in the dataverse context up until now, but it was worth it: I have learned a lot). Now both scripts; preparation and test, run without problems. Can you see why the jenkins build fails? I cannot see the output of that. The output of the integration test that I get locally looks OK to me:

[INFO] Results:
[INFO] 
[WARNING] Tests run: 185, Failures: 0, Errors: 0, Skipped: 8
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  19:48 min
[INFO] Finished at: 2022-11-16T15:44:53+01:00
[INFO] ------------------------------------------------------------------------

@kcondon
Copy link
Contributor

kcondon commented Nov 16, 2022

Hi @ErykKul The jenkins build fails due to a component in the test pipeline infrastructure and not due to an integration test. Essentially, the image used for the test box has been updated and we are waiting for the corresponding ansible lib to be able to host the tests. The team is looking at it now. Thanks for the efforts and apologies for the delay.

@qqmyers
Copy link
Member

qqmyers commented Nov 16, 2022

The build issue is related to gdcc/dataverse-ansible#263 and is not about a failing test. Once that issue is fixed (Don is working now), rerunning the build job should be enough to get the build to show green. So as far as I can tell, nothing for @ErykKul to do now (except wait a bit).

@ErykKul
Copy link
Collaborator Author

ErykKul commented Nov 16, 2022

OK, thanks!

@qqmyers
Copy link
Member

qqmyers commented Nov 18, 2022

@ErykKul FYI - tests have run and there's a new failure:

java.lang.AssertionError: 
Expected status code <204> doesn't match actual status code <400>.
	at edu.harvard.iq.dataverse.api.SwordIT.testDeleteFiles(SwordIT.java:863)

@kcondon kcondon merged commit 948c0d0 into IQSS:develop Nov 21, 2022
@pdurbin pdurbin added this to the 5.13 milestone Nov 21, 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.

Sorting of licences by admins Multiple license sorting is inconsistent
6 participants