-
Notifications
You must be signed in to change notification settings - Fork 11
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
LBAC-33 :Add one or more access locations to a user #57
Conversation
Did you build the module and test these changes and found them working perfectly? |
@HerbertYiga work on them need to make some changes in this to allow the user to login with multiple locations |
Can you deploy the changes to https://modules-refapp.openmrs.org , and let me know to test it? |
78a687d
to
ef484c1
Compare
@VANKINEENITAWRUN can you include the ticket id in your commit message as advised at? https://wiki.openmrs.org/display/docs/Pull+Request+Tips |
ef484c1
to
edf7dca
Compare
edf7dca
to
a44b7a6
Compare
One major concern here, In your ticket you have mentioned to work only for users. But this PR contains a lot of changes in other services (Patient/Encounter/..etc). Did you verify that all other features of this module can work without any issues regarding these changes? |
api/src/main/java/org/openmrs/module/locationbasedaccess/utils/LocationUtils.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/locationbasedaccess/utils/LocationUtils.java
Show resolved
Hide resolved
Yes, As the User is Having multiple Locations Access, we get a list of Locations in every InterceptorAdvice of the User so there should be filtered accordingly using the List. |
@suthagar23 any more comments for this? |
@dkayiwa Still some comments need to be addressed! @VANKINEENITAWRUN Can you check the comments and follow JIRA guidelines also. |
a44b7a6
to
a393fc6
Compare
@suthagar23 fixed Merge Conflicts, can you please mention JIRA guidelines which I got missed so that I can catch up with them from next time. |
@suthagar23 what do you think of this? |
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.
LGTM
Description of what I changed
ServiceInterceptorAdvice
to remove those objects which do not belong to User's Location List.Issue I worked on
see https://issues.openmrs.org/browse/LBAC-33
Checklist: I completed these to help reviewers :)
My pull request only contains ONE single commit
My IDE is configured to follow the code style of this project.
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
I ran
mvn clean package
right before creating this pull request andadded all formatting changes to my commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.