Skip to content

Conversation

@Daetha
Copy link
Collaborator

@Daetha Daetha commented Jan 13, 2026

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

  • My code follows the style guide.
  • I have self-reviewed my code.
  • I added comments for hard-to-understand code.
  • If applicable, PHP code is documented using PHPDoc.
  • If applicable, JavaScript code is documented using JSDoc.
  • If needed, the ELMO Guide has been updated.
  • If needed, the README has been updated.
  • If needed, the API documentation has been updated.
  • If a new feature was added or a bug fixed, the changelog has been updated.
  • My changes do not create new warnings in the test browser console.
  • I have added unit tests that cover my code.
  • All new and existing unit tests pass locally.
  • All new and existing automated unit tests pass in the pull request.
  • If applicable, Playwright tests have been updated and new tests added.
  • All new and existing automated Playwright tests pass in the pull request.
  • I ensured the changes meet accessibility guidelines.

Known Issues

oops, we should have had ERNIE API in the loop here. see #740

@Daetha Daetha self-assigned this Jan 13, 2026
@Daetha Daetha requested a review from McNamara84 January 15, 2026 23:06
Copy link
Owner

@McNamara84 McNamara84 left a 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.js Functions. 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.js Functions. 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!

@Daetha
Copy link
Collaborator Author

Daetha commented Jan 23, 2026

  • setupLicenseDropdown() is function defined twice

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 function setupLicenseDropdown(). dwells exactly 1 result...

@Daetha
Copy link
Collaborator Author

Daetha commented Jan 26, 2026

  • 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.

Based on my research only the GPL license is copyleft among the 4, so it is displayed last, after the permisive licenses

@Daetha Daetha requested a review from Ali-GeoFZ January 30, 2026 16:32
@Daetha
Copy link
Collaborator Author

Daetha commented Jan 30, 2026

Dear @Ali-GeoFZ, this PR is ready for review : )

@Ali-GeoFZ
Copy link
Collaborator

Hey @Daetha, the tests in this pull request didn't pass. Should I still review it?

@Daetha
Copy link
Collaborator Author

Daetha commented Feb 3, 2026

Hey @Daetha, the tests in this pull request didn't pass. Should I still review it?

Turns out not yet : )
Sorry for the false ping

Copy link
Owner

@McNamara84 McNamara84 left a 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.js again and
  • JSDoc comments in help.js too.

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!

Copy link
Owner

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() {
Copy link
Owner

Choose a reason for hiding this comment

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

Missing JSDoc documentation.

Copy link
Collaborator

@Ali-GeoFZ Ali-GeoFZ left a 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.

@Daetha Daetha requested a review from Ali-GeoFZ February 10, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize license display by resource type 4 better orientation

3 participants