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

Multiple license sorting is inconsistent #8554

Closed
Kris-LIBIS opened this issue Mar 30, 2022 · 10 comments · Fixed by #8567 or #8697
Closed

Multiple license sorting is inconsistent #8554

Kris-LIBIS opened this issue Mar 30, 2022 · 10 comments · Fixed by #8567 or #8697
Milestone

Comments

@Kris-LIBIS
Copy link
Contributor

What steps does it take to reproduce the issue?
Using Dataverse >= 5.10, install multiple licenses. The licenses will be sorted in the order they are loaded.
Then make one of the licenses the default or change active to false and back to true using the API.
The order of the licenses will be changed. Probably the last one you modified using the API will appear last in the drop-down list.

  • When does this issue occur?
    When a license is changed via API or even when editing the database table directly.

  • Which page(s) does it occurs on?
    The license selection in the Terms page

  • What happens?
    The order of the licenses in the drop-down is changed.

  • To whom does it occur (all users, curators, superusers)?
    Any installation using multiple licenses

  • What did you expect to happen?
    The licenses sorting order should remain the same or at least should be consistent.

Which version of Dataverse are you using?
5.10

Any related open or closed issues to this bug report?

Screenshots:

No matter the issue, screenshots are always welcome.

To add a screenshot, please use one of the following formats and/or methods described here:

@Kris-LIBIS
Copy link
Contributor Author

It seems that the licenses list is populated by a SQL statement that does not include a ORDER BY clause and hence the licenses are displayed in whatever order they are physically stored in the database. Changing any field of a record can result in a change of the physical location of that record and would explain the difference in sorting.

I see multiple ways to address this issue:

  1. Always sort by ID. The order will be consistent and fixed to the order the licenses are added.
  2. As above, but always list the default license at the top, similar to how the 'Custom Terms' option is always on the bottom.
  3. Add a order field that allows administrators (via API) to determine the order of the licenses themselves.

@pdurbin
Copy link
Member

pdurbin commented Mar 30, 2022

@Kris-LIBIS thanks for digging in and checking that there isn't already an ORDER BY clause (I haven't looked myself).

Rather than ordering by ID (a number), I suppose it would make more sense to order by the human readable name. (I believe this is the name field.)

That said, yes, I agree it would be nice to put the default license at the top.

I'm not sure what to say about "Custom Terms". It sounds like you're saying it's always on the bottom. I guess this is fine.

@jggautier
Copy link
Contributor

Yeah having the default license always appear first was the intention, since I think that determines what the default is, right?

And having Custom Terms always appear last was intentional, too.

@Kris-LIBIS
Copy link
Contributor Author

@pdurbin TBH, I did not check the code at first. But not having an ORDER BY is what would explain why the order of the licenses changes. Looking at this file (is this the correct file?) seems to confirm that.

For instance, we loaded 15 new licenses and they showed up in loading order beneath the default CC0, as we expected. Then I changed the CC0 URI from http to https in the database and CC0 suddenly appeared last (technically it is the penultimate because 'Custom Dataset Terms' is always appended as the last option).

@jggautier Next, I set license nr 2 (our first custom license) as the default and then that one suddenly appeared on the bottom.

Regarding sorting by name, that is definitely not what our organization wants. Our list of licenses is a combination of the typical data licenses like the Creative Commons suite and code licenses like GPL, AGPL and BSD. It is a requirement that the data licenses and code licenses are not intermixed and that is exactly what sorting by name would do.

I suggested sorting by ID because it is the easiest way to implement a predictable order for the licenses without changing the data model.

It would also mean that you can only append to the bottom of the list at a later stage. If that is an issue, option 3 would be a better solution.

@jggautier
Copy link
Contributor

jggautier commented Mar 30, 2022

Thanks so much for all the details and thought! I think option 3 would be best. It seems way simpler and more explicit than the first two options, although I can't speak to how much work that would be.

And for option 2, like I think you said, controlling the order that the options appear in (except for the default option and the custom terms option) would require being careful about the order in which the options are added over time. But it would at least make the order consistent each time someone clicks on the dropdown and ensure that the default option is listed at the top and the custom terms option is listed at the bottom.

Is it technically possible to have the custom terms option be the default? I'll try to look over notes for any discussion about this but thought I'd ask here too.

Edit: I looked over notes and mockups, like the proposal that summarized the goals, and so far it looks like it was never explicitly stated that custom terms could not be set as the default. But I don't think we're aware of any use-cases where a repository would want the custom terms option to be the default, and if we wanted to support that, we'd have to adjust the deposit workflow in one of several ways, for example so that people can edit Terms metadata when they first create a dataset, since choosing the custom terms option makes the Terms of Use field required.

Anyway, that's all to say that I wouldn't expect us to be able to set the custom terms option as the default.

@Kris-LIBIS
Copy link
Contributor Author

@jggautier option 2 would be very easy to achieve. It's merely a matter of adding 'ORDER BY isdefault desc, id asc' to the SQL statement(s) that retrieves the list of licenses.

In order to implement option 3 we would need flyway scripts to add the extra column to the table and to populate that column if empty. We would also need to add the order value to what is returned by the APIs that return license info and a new API that allows an admin to change the order value of a given license. Maybe we will need to add some unit tests as well? So, definitely more work, yes.

As to setting the custom terms option as the default: no, I don't think you can do that currently. The issue is that you can set a certain license as the default by giving the API the id of the license. But the custom terms option is not a license that is part of the license table. This code seems to be adding the custom terms option to the drop down list only when the drop down is rendered.

However, we do support your idea on the deposit workflow change. Our organization did not want the RDM to have a default license - why, that is a long story. But it is currently mandatory to have a default license, so we ended up creating a dummy license that asks the user to select a license and made that the default. Our reviewers will need to check that the submitter did indeed select one of the other licenses before the dataset can be published. It's a bit of a hassle, but is acceptable as a workaround. It would help a lot to have the dataset creation workflow force the submitter to edit the Terms info before the dataset can be saved. The workflow would be similar to what happens when you create a new dataset template. I think we were planning to create an issue for that.

@jggautier
Copy link
Contributor

jggautier commented Mar 31, 2022

Thanks for opening #8561. I wouldn't say that what I wrote was in support of the idea of Deposit workflow changes, more just including reasons why the custom terms option couldn't be the default option, although the discussion in #8561 is also very insightful.

About option 2, what would be the solution if you needed to add a new license to the dropdown but didn't want that new license at the end of the list? Or does your repository not expect to need to add new licenses?

Would it be helpful to think of option 2 as a good first step since it's technically easy to implement and definitely improves how things work now, since it'll at least make the order consistent? Then other options that might be more work can continue to be explored?

@Kris-LIBIS
Copy link
Contributor Author

@jggautier Yes, you are right. Option 2 would always append new licenses to the end. That's why I proposed an alternative solution as option 3. That would definitely be the better option.

I went ahead and created a PR #8567 that implements option 2 as a preliminary fix. We need this ourselves anyway to be able to roll-out the multiple license feature for internal testing. Implementing option 3 should include a similar code change.

@qqmyers
Copy link
Member

qqmyers commented Apr 1, 2022

An aside w.r.t. changing licenses: The discussion @philippconzett is leading to standardize the name/URLs for licenses is relevant - by changing the CC0 1.0 license to use https:// in it's URL, it won't be recognized in other instances as the same license.

@pdurbin pdurbin added this to the 5.11 milestone Apr 8, 2022
@pdurbin
Copy link
Member

pdurbin commented Apr 8, 2022

@Kris-LIBIS the pull request you created...

... was just merged and closed this issue automatically (with "close" keyword).

And really, it makes sense, given the title. Sorting should now be consistent, now that we're sorting by id.

If you (or anyone!) would like a further improvement (a new "sort order" database column, for example), please open a fresh issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants