Skip to content

Conversation

@ddey2
Copy link
Member

@ddey2 ddey2 commented Mar 8, 2024

  1. When you create a dataset, there should be a new step content for adding a license. A license can be either a standard one or a custom license.
  2. If you choose the standard license, you can view the name of the license and click on it to see more details of the license when you're on the dataset page.
  3. If you choose the custom license, a modal will open. You can fill in the details and it will be created once you click on 'Finish button'. On the dataset page, you should also see an 'Edit icon button' , on clicking it should open the edit license modal.

PFA screenshots for reference.
Screenshot 2024-03-11 at 1 58 59 PM
Screenshot 2024-03-11 at 1 59 10 PM
Screenshot 2024-03-11 at 1 59 35 PM
Screenshot 2024-03-11 at 1 58 34 PM

@ddey2 ddey2 requested review from lmarini and tcnichol March 8, 2024 04:04
@ddey2 ddey2 mentioned this pull request Mar 8, 2024
@tcnichol
Copy link
Contributor

I'm getting an error when I try to add a custom license. Here is the error I'm seeing

File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__ pydantic.error_wrappers.ValidationError: 1 validation error for LicenseDB name field required (type=value_error.missing)

Copy link
Contributor

@tcnichol tcnichol left a comment

Choose a reason for hiding this comment

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

Tested. Everything works, including custom license which I can edit after adding.

@ddey2 ddey2 requested a review from tcnichol March 13, 2024 12:49
@ddey2
Copy link
Member Author

ddey2 commented Mar 13, 2024

@tcnichol I added the logo, Could you please retest it?
Now, everywhere I have replaced the standard license names with a logo if available.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

image

A few additional things I noticed:

  • For holders, are we freely allowing anyone (username? full name? email?) to be the holder or they need to be clowder user? @lmarini
image - After creating a custom licnece, then I went in edit for the first time (without changing the date time), i got above error. - if i change anything on the datetime, it passed and it never happened again

@ddey2
Copy link
Member Author

ddey2 commented Mar 14, 2024

image A few additional things I noticed:
  • For holders, are we freely allowing anyone (username? full name? email?) to be the holder or they need to be clowder user? @lmarini

image - After creating a custom licnece, then I went in edit for the first time (without changing the date time), i got above error. - if i change anything on the datetime, it passed and it never happened again

@longshuicy I am not able to reproduce this one.

@lmarini lmarini requested a review from longshuicy March 21, 2024 14:11
@lmarini lmarini changed the base branch from 687-license-support to main March 21, 2024 14:18
Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Works well now. Thank you!

@ddey2 ddey2 requested a review from longshuicy March 22, 2024 17:16
Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Approved.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Sorry 2 more things I just noticed. But feel free to address in another issue:

  1. The selecting licence part seems to be optional. Is that intended deisgn? @lmarini
image
  1. if nothing is set, could we have some default text saying "no licence?". Right now it's showing a broken link
image

@lmarini lmarini merged commit 8fa614b into main Apr 12, 2024
@lmarini lmarini deleted the 946-list-standard-licenses branch April 12, 2024 21:18
@lmarini
Copy link
Member

lmarini commented Apr 12, 2024

Sorry 2 more things I just noticed. But feel free to address in another issue:

1. The selecting licence part seems to be optional. Is that intended deisgn? @lmarini
image
2. if nothing is set, could we have some default text saying "no licence?". Right now it's showing a broken link
image

Sorry @longshuicy @ddey2 I clicked merge too quickly! We can definitively do this on a separate PR.

I think we shouldn't let them leave license blank.

@longshuicy
Copy link
Member

Sorry 2 more things I just noticed. But feel free to address in another issue:

1. The selecting licence part seems to be optional. Is that intended deisgn? @lmarini
image ``` 2. if nothing is set, could we have some default text saying "no licence?". Right now it's showing a broken link ``` image

Sorry @longshuicy @ddey2 I clicked merge too quickly! We can definitively do this on a separate PR.

I think we shouldn't let them leave license blank.

No worries. I meant to open new issue and will do :-)

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.

Add an option for custom license Add a list of standard licenses options

5 participants