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

Feature/limited rights #89

Merged
merged 7 commits into from
Dec 9, 2024
Merged

Feature/limited rights #89

merged 7 commits into from
Dec 9, 2024

Conversation

J4bbi
Copy link
Collaborator

@J4bbi J4bbi commented Nov 21, 2024


This PR limits the availability of licenses for the user to choose from in the submission form. The full list of 419 licenses is taken from the SPDX list of licenses is to be found in invenio-rdm-records.

The standard functionality is to allow multiple licenses and custom licenses. This is done by providing an "Add custom" button, as well as an "Add standard" button. Clicking the "Add standard" button open ups a search modal that searches the api/vocabularies/licenses endpoint using the react-searchkit library.

This PR creates three new/modifies components: LimitedLicenseFieldItem, LimitedLicenseModal and LimitedLicenseField.

  • The Add custom button is omitted
  • The Add standard button is renamed to Add license
  • The Add license button is only displayed if the number of licenses is falsy
  • The name of the Edit button of an already selected license is renamed to Change
  • The search bar and the filtering by tags (Recommended, All, Data and Software) are omitted
  • The initialQueryState of the react-searchkit searchConfig is filter by the data tag, given a query to search by id and limited to two results

The backend endpoint is of course untouched.

Developer Checklist

Developers should review and confirm each of these items before requesting review

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Code documentation and related non-code documentation has all been updated

Reviewer Checklist

Reviewers should review and confirm each of these items before approval
If there are multiple reviewers, this section can be duplicated for each reviewer

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested

Testing

List user test scripts that need to be run

List any non-unit test scripts that need to be run

@J4bbi J4bbi requested a review from cc-a November 21, 2024 16:56
Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

Thanks @J4bbi. In terms of the resulting UI I think this is good. I'm slightly conflicted on the implementation tbh.

I can see that this was the shortest possible route to get from the existing license field to the behaviour we want but the end result is that for what we get (a modal with two buttons give or take) the code seems overly complex.

I'm aware that your guys time is finite so lets go ahead with this implementation for now (after addressing the comments I've added). I'm going to add an item to the backlog to explore simplifying the implementation down the line.

This is a potentially naive question but could a select input do the job?

@J4bbi J4bbi force-pushed the feature/limited_rights branch from 6b82f4c to 6231cb9 Compare December 9, 2024 09:57
@J4bbi
Copy link
Collaborator Author

J4bbi commented Dec 9, 2024

Thanks @J4bbi. In terms of the resulting UI I think this is good. I'm slightly conflicted on the implementation tbh.

I can see that this was the shortest possible route to get from the existing license field to the behaviour we want but the end result is that for what we get (a modal with two buttons give or take) the code seems overly complex.

I'm aware that your guys time is finite so lets go ahead with this implementation for now (after addressing the comments I've added). I'm going to add an item to the backlog to explore simplifying the implementation down the line.

This is a potentially naive question but could a select input do the job?

Thanks @cc-a

I found it tricky to replicate the functionality of the current component, that would also have required providing the hard-coded licenses and not use the search index.

@J4bbi J4bbi requested a review from cc-a December 9, 2024 10:10
Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

Thanks @J4bbi. Looks good.

# Conflicts:
#	assets/js/invenio_app_rdm/overridableRegistry/mapping.js
@cc-a cc-a enabled auto-merge December 9, 2024 16:10
@cc-a cc-a merged commit 49ca923 into develop Dec 9, 2024
2 checks passed
@cc-a cc-a deleted the feature/limited_rights branch December 9, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants