Skip to content

Change description of admin role #3189#8666

Merged
kcondon merged 1 commit intoIQSS:developfrom
konradperlowski:3189_admin_role_description
Apr 29, 2022
Merged

Change description of admin role #3189#8666
kcondon merged 1 commit intoIQSS:developfrom
konradperlowski:3189_admin_role_description

Conversation

@konradperlowski
Copy link
Contributor

@konradperlowski konradperlowski commented Apr 29, 2022

What this PR does / why we need it:

changes admin role description - specifying that admin role can approve request to restricted data

Which issue(s) this PR closes:

Special notes for your reviewer: None

Suggestions on how to test this:

Check if description is valid

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

In the permissions section, description of admin role was changed:
image

Is there a release notes update needed for this change?: No

Additional documentation: None

@TaniaSchlatter TaniaSchlatter removed their request for review April 29, 2022 14:19
@TaniaSchlatter
Copy link
Member

Looks good!

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

In Slack both @sbarbosadataverse and @TaniaSchlatter approved the wording change: https://iqss.slack.com/archives/CVB2SMDFX/p1651231790124739

I'll go ahead and approve this.

@konradperlowski please don't worry about the "docs/readthedocs.org:dataverse-guide" check failing. This is new and should stop failing once we merge PR #8651.

For other developers I'd like to point out that we're storing the description of roles in two places (now that PR #5158 has been merged), which seems a bit suboptimal to me.

Changing src/main/java/propertyFiles/BuiltInRoles.properties as this pull request does is the right thing to do. The old description is in the database...

dvndb=> select name,description from dataverserole where name = 'Admin';
 name  |                              description                              
-------+-----------------------------------------------------------------------
 Admin | A person who has all permissions for dataverses, datasets, and files.
(1 row)

... but it's in English and seems to be treated in the code as a fallback:

public String getDescription() {
    if (alias != null) {
        String key = "role." + alias.toLowerCase() + ".description";
        try {
            String _description = BundleUtil.getStringFromPropertyFile(key, "BuiltInRoles");
            if (_description == null) {
                return description;
            } else {
                return _description;
            }

I doubt the database version is used very often. And it would be a bad experience for non-English installations. At some point we should probably drop the description field from the dataverserole table. (And blank out the description string from scripts/api/data/role-admin.json.) For now, I think we should simply allow BuiltInRoles.properties to be the source of truth and not worry about what's in the database.

@kcondon kcondon self-assigned this Apr 29, 2022
@kcondon kcondon merged commit c47cf82 into IQSS:develop Apr 29, 2022
@pdurbin pdurbin added this to the 5.11 milestone May 4, 2022
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.

Permissions: update "roles" description for "Admins" to specify this role allows you to approve requests to restricted data

4 participants