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

DCM-56: Add user privileges to Location Mapping UI #47

Merged
merged 1 commit into from Aug 7, 2021

Conversation

Piumal1999
Copy link
Contributor

@Piumal1999 Piumal1999 commented Jul 27, 2021

The issue I worked on

See https://issues.openmrs.org/browse/DCM-56

Description of what I changed:

Updated the Location Mapping UI to behave according to the user privileges

Related Thread and Preview:

https://talk.openmrs.org/t/dhis-connector-module-user-access-controlling/34059/5?u=piumal1999

@Piumal1999 Piumal1999 marked this pull request as ready for review July 29, 2021 15:58
@akshika47
Copy link
Collaborator

@jayasanka-sack would you be able to take a look at this PR?

</c:forEach>
</select>
</td>
<openmrs:hasPrivilege privilege="View Location Mappings,Manage Location Mappings">

Choose a reason for hiding this comment

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

thanks @Piumal1999 on ensuring that the person who accesses this page has the required privileges as per your first line,do you really need to re check again at this point for the privileges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right @HerbertYiga. I'll remove that line. Thanks for suggesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with d673db3

Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

@Piumal1999 I have pointed out a couple of questions.
Could you please check them?

Comment on lines +3 to +4
<openmrs:require anyPrivilege="View Location Mappings,Manage Location Mappings" otherwise="/login.htm"
redirect="/module/dhisconnector/locationMapping.form"/>
Copy link
Member

Choose a reason for hiding this comment

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

What if the user is already logged in, but doesn't have the above permissions? Then redirecting them into the login page will redirect back to the home page. I think it's better if we could make it more informative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the user is already logged in, but doesn't have the above permissions? Then redirecting them into the login page will redirect back to the home page

yeah @jayasanka-sack. I used the '/login.htm' page because it's the only page that anyone can access. However, the links for these pages are not visible in the navbar. So this redirection will only happen if someone tries to access the page by finding the URL somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, then that's fine.

@@ -35,7 +37,8 @@
</c:forEach>
<input name="savedOrgUnitUuidOf_${location.uuid}" type="hidden" value="${savedOrgUnitUuid}">
<td>
<select name="orgUnitOf_${location.uuid}">
<select name="orgUnitOf_${location.uuid}"
<openmrs:hasPrivilege privilege="Manage Location Mappings" inverse="true">disabled</openmrs:hasPrivilege>>
Copy link
Member

Choose a reason for hiding this comment

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

It seems the user cannot access this page if the user doesn't have the Manage Location Mappings permission. So this is an unreachable code I guess.

Copy link
Member

Choose a reason for hiding this comment

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

I went through it again. Ignore this comment.

Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

Thanks for getting this done, @Piumal1999 !

@jayasanka-sack jayasanka-sack merged commit d3af791 into openmrs:master Aug 7, 2021
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