Skip to content

5126 json built in roles#5158

Merged
kcondon merged 25 commits intoIQSS:developfrom
scholarsportal:5126_JSON_BuiltInRoles
Oct 29, 2018
Merged

5126 json built in roles#5158
kcondon merged 25 commits intoIQSS:developfrom
scholarsportal:5126_JSON_BuiltInRoles

Conversation

@JayanthyChengan
Copy link
Contributor

connects to #5126

@coveralls
Copy link

coveralls commented Oct 9, 2018

Coverage Status

Coverage decreased (-0.005%) to 15.624% when pulling d6420ca on scholarsportal:5126_JSON_BuiltInRoles into 2bac293 on IQSS:develop.

admin.api.migrateHDL.failure=Failed to migrate Dataset Handle id: {0}
admin.api.migrateHDL.failureWithException=Failed to migrate Dataset Handle id: {0} Unexpected exception: {1}

#Roles
Copy link
Contributor

Choose a reason for hiding this comment

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

These are actually permissions; let's rename them that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced we want to translate these - though yes to their descriptions. (it's probably ok to translate, it's just these are "reserved words" in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will discuss with @amberleahey and @mhvezina to clarify whether those "reserved words" from line number 2074 to 2086 to be translated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since those strings are displayed in the user interface, I would like to have a way to be able to translate them.

Choose a reason for hiding this comment

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

I too agree we should translate these: they may be role types that are stored but they are readable/meaningful in the user interface. This is working as per the updated pull request
customroles

roles.DeleteDataverse=DeleteDataverse
roles.DeleteDatasetDraft=DeleteDatasetDraft

permission.AddDataverse=Add a dataverse within another dataverse
Copy link
Contributor

Choose a reason for hiding this comment

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

and this is their descriptions.

if (alias != null )
{
String key = "role." + alias.toLowerCase() +".name";
return BundleUtil.getStringFromPropertyFile(key,"BuiltInRoles" );
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work with Custom roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scolapasta: I didn't handle custom role in this pull request. Is custom role also comes from JSON files? Haven't investigated on the logic behind custom role. Please add some information related to custom roles, can we handle it in separate issue?

Choose a reason for hiding this comment

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

After testing the fix @JayanthyChengan made today, custom roles in our local dvdev is now working as expected, can create/edit/apply custom roles.
customroles
applyingmytestrole

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scolapasta: the code got outdated after I committed new code( which will handle custom roles). Hope I understood correctly and updated the code accordingly. Please review

@JayanthyChengan
Copy link
Contributor Author

renamed the property in bundle.properties as @scolapasta requested.

@djbrooke
Copy link
Contributor

Thanks @JayanthyChengan. Is this ready for code review?

@JayanthyChengan
Copy link
Contributor Author

Yes, it's ready for code review

@kcondon kcondon merged commit 8f95265 into IQSS:develop Oct 29, 2018
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.

8 participants