-
Notifications
You must be signed in to change notification settings - Fork 500
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
7814: sorting users in the dashboard #7815
7814: sorting users in the dashboard #7815
Conversation
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.
Overall changes seem stragihtforward; we'll spin upa branch to have UI/UX take a quick look.
That said, @pkiraly, in the future, could you be careful about introducing small format changes (spaces, tabs ,etc) - they make it very hard to review as there are several of thoe changes, even in files that would otherwise not be included as part of this PR.
If you think these formatting changes are useful (and I do appreciate the extra spaces are an if (x = y) clause, for example), then we could introduce them in separate, dedicated PRs, or maybe just a few here or there. In this case, it took me a lot longer to review, just to make sure the changes that github reported weren't actually functional, when the actual functional changes were actually quite small.
Just a reminder that I added a Checkstyle config file in pull request #5106. We don't use it as part of our build process but we could. Some docs here: https://guides.dataverse.org/en/5.4.1/developers/coding-style.html#checking-your-formatting-with-checkstyle |
@scolapasta I got it, and will follow this rule. Sorry for the inconveniences. I have a question though: when I started contributing to Dataverse one of my first PRs was a large code format change request. It was refused for two reasons: (1) it was too much in one step, and (2) code format changes are on a very low level on the priority list, so these kind of changes has been not welcomed at that time (about two years ago). Has this policy been changed? Are small code format PRs allowed? (It is my weakness, but when I check a code, and formatting problems hijack my focus from the the business logic.) |
Good question and I'm not sure I have a great answer. I'd say, they would be lowish priority, but if they really are non functional changes, I don't see why we wouldn't take them if they improve general code readabilty. But I would suggest erring on the small side. For example, once thing could be, if it's going to be like this change, do it as two PRs, one for formatting changes in these files, 2 for the functional change. That way the functional change can be evaluated for its merit, and the style change is smaller in scope since it relates. Just an idea. A lot of times it will come down to priority and bandwidth. |
@scolapasta Thanks for the explanation! |
@pkiraly Found one minor issue, for Last Name, Z-A order comes before A-Z, unlike all the other fields. Is this intentional? It seems sorting by authentication method also isn't working but with no server log error. So, 2 issues, 1 dependency:
Dependency:
|
…tion and improve API's documentation.
@pkiraly Thanks! All issues are resolved. Now waiting the other issue. |
This PR was developed by Göttingen eResearch Alliance, Germany, and funded by SSHOC, "Social Sciences and Humanities Open Cloud". SSHOC has received funding from the European Union’s Horizon 2020 project call H2020-INFRAEOSC-04-2018, grant agreement #823782. |
What this PR does / why we need it: It is not possible to sort users in the Dashboard. As a system administrator I would like to check the latest newcomers via sorting by user ID. The Dataverse API makes it possible for a while, but it was not implemented in the user interface.
Which issue(s) this PR closes: #7814
Closes #7814
Special notes for your reviewer: The PR contains new entries in the Bundle.properties. In the discussion with @qqmyers on Dataverse Dev mailing list we mentioned that PrimeFaces provides sortable and lazy data tables. I tested them, and I do not feel that they work as expected -- they sort not the whole database table only the actual view of it. Moreover Dataverse supports PrimeFaces 8.x, while current PrimeFaces showcases are based on 10.x or 11.x, and the lazy datatable API has been changed a lot.
I am not a UI developer, so maybe the final placement and rendering of the sort dropdown list should be changed. I think however at least the main part of the backend side could be acceptable.
Suggestions on how to test this: Since this change mainly contains a user interface change, you have to build the war file, deploy, and create some test users. The log in as administrator, go to Dashboard > Manage users page, and check the "Sort" button.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
![7814](https://user-images.githubusercontent.com/218218/115620587-a098fc80-a2f5-11eb-8b90-d31e0f9c7725.png)
Is there a release notes update needed for this change?: It might be mentioned, that now site admins can sort the user list.
Additional documentation: no