-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: improve user choice of licenses #895
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
base: main
Are you sure you want to change the base?
Conversation
… changes in resource type
removed license tests from autocomplete tests.
McNamara84
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @Daetha ,
I don't understand why this has been prioritized for the current sprint, because ERNIE already has the endpoint /api/v1/licenses/elmo, and I am also happy to add further endpoints to ERNIE (such as / api/v1/licenses/elmo-msl and /api/v1/licenses/elmo-gem) to ERNIE or add further features such as software licenses. Then K. could add new licenses or deactivate existing licenses himself in the future. But anyway...
I can not approve this changes because of the following points:
- The issue #875 (Duplicate entries in the License dropdown) requires a Playwright test that ties to reproduces the bug, but the PR doesn't add such a test. The acceptance criteria explicitly states that point.
setupLicenseDropdown()is function defined twice- I am missing Unit Tests for New
help.jsFunctions. Is'nt the coverage of new and edited code on your definition of done anymore? - For non-software licenses: CC-BY-4.0 is placed first, rest alphabetically. For software licenses: GPL-3.0-or-later is placed LAST, rest alphabetically. But the PR description mentions: "First, non-copyleft licenses in alphabetical order, then copyleft licenses" but the implementation only moves GPL to the end, not all copyleft licenses. This is unclear for me. Maybe wrong implementation or wrong documented...
- I am missing JSDoc for the new
help.jsFunctions. Have you changed the definition of done for the ELMO project?
Can you please make a own review before requesting a re-review from me. It helps to check the checklist in the PR description to check if definition of done ist fullfilled. Thanks!
Dear Holger, thank you for the review! Let me reply to your points first and then I will adress the ERNIE part. setupLicenseDropdown() is function defined once, I think. Searching for |
Based on my research only the GPL license is copyleft among the 4, so it is displayed last, after the permisive licenses |
|
Dear @Ali-GeoFZ, this PR is ready for review : ) |
|
Hey @Daetha, the tests in this pull request didn't pass. Should I still review it? |
Turns out not yet : ) |
McNamara84
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is looking good, but is missing
- code coverage for
help.jsagain and - JSDoc comments in
help.jstoo.
Anyway, I am approving this PR because I do not know if the Definition of Done has changed in the last week. @Ali-GeoFZ will see this comment because he will do the functionallity tests and can comment about this topic.
Please do not merge before @Ali-GeoFZ is done with the functionalltity tests. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new functions in this file have low code coverage with unit tests.
| $('.input-with-help').toggleClass('input-right-with-round-corners', status === 'help-off'); | ||
| } | ||
|
|
||
| function getSelectedResourceType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JSDoc documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear Ali! thank you for the review.
Let me adreess your points here:
License list per Resource Type
When I select a different type, I still see only the Dataset licenses instead of all licenses. Is this intentional or a bug?
This is intentional. Some Licenses are only for software and don't apply to any other resource types. But ERINE can do it better: it is planned that the curators will be able to choose the set of licenses for each of resource types.
when selecting “Choose”
If I switch the Resource Type back to “Choose” (no selection), the License dropdown should reset and display all licenses. Currently, it keeps showing the last filtered list instead.
Regarding the "Choose" option:
I understand your suggestion, but adding it would actually hurt the user experience:
The UX problem: License selection comes last because it depends on resource type. If we show a "Choose" option, users will select it, then we'll override their choice when they pick a resource type. This is frustrating – why show an option we won't honor? For scientists focused on their work, this creates unnecessary confusion.
Our current approach – pre-selecting the appropriate license based on resource type – is simpler, clearer, and more reliable for both users and developers.
Minor note: I currently don’t clearly see the alphabetical sorting
The sorting is not fully alphabetical: The copyleft licenses mmust be displaced the last, CC-BY-4 -- must be first.
closes #887 and #888 and #875
Changes
applied sorting for software/not-software licenses
applied a custom sorting to keep cc-by-4.0 on top, others alphabetically
moved the FG to the last most group
dynamically update the help section based on the trsource type chosen
Notes for Reviewer
the API links for licenses in ERNIE:
https://ernie.rz-vm182.gfz.de/api/v1/licenses/elmo/software
https://ernie.rz-vm182.gfz.de/api/v1/licenses/elmo/dataset
Checklist
Known Issues
oops, we should have had ERNIE API in the loop here. see #740