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

LBAC-2 Added implementation to add the locations to the users on registration #17

Merged
merged 1 commit into from Jul 24, 2018

Conversation

@suthagar23
Copy link
Member

@suthagar23 suthagar23 commented Jul 20, 2018

Description

This PR contains the implementation to assign the location to the users on registration. It contains two custom fragments for,

  • Add/Edit the location in the user dashboard
  • View the location information in the user dashboard

Ticket

Ticket : https://issues.openmrs.org/browse/LBAC-2

@@ -13,4 +13,5 @@
public class LocationBasedAccessConstants {

public static final String LOCATION_ATTRIBUTE_GLOBAL_PROPERTY_NAME = "locationbasedaccess.locationAttributeUuid";
public static final String USER_PROPERTY_LOCATION_ATTRIBUTE_NAME = "locationUuid";

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Is the above constant name really correct?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

changed to "LOCATION_USER_PROPERTY_NAME"

@@ -1,3 +1,4 @@
${project.parent.artifactId}.title=Location based access control module

locationbasedaccess.patientLocation=Patient Location

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Are we still using locationbasedaccess.patientLocation for anything?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

For showing the label on the patient registration dashboard.

"type": "userProperty",
"userPropertyName" : "locationId",
"label": "Select Location",
"uuid": "8b5c95ef-103c-41bc-9f24-368b8f77e070",

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

What is the above uuid for?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

removed

"title" : "Location",
"type": "userProperty",
"userPropertyName" : "locationId",
"label": "Select Location",

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Shouldn't we localise these labels?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Yes, we need to do it. Since other extensions also need to have it, Can we do these all with a different ticket?

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

I thought you would just replace them with message codes hence not requiring a separate ticket.

public class ViewUserLocationFragmentController {

public void controller(FragmentModel model, FragmentConfiguration config) {
Object userId = config.getAttribute("userId");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Did you intentionally decide to use the integer userId instead of user uuid?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Just updated

if (userId != null) {
User user = Context.getUserService().getUser(Integer.parseInt(userId.toString()));
if (user != null) {
String locationProperty = user.getUserProperty("locationId");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Shouldn't the property be locationUuid?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Just updated with constants

}
else if (person != null) {
Location location = LocationUtils.getPersonLocation(person);
if(location!=null) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

The formatting would always be better if you take advantage of spaces. For instance the above would be like:
if (location != null) {

model.addAttribute("selectedLocationUuid", patientLocation.getUuid());
}
Location location = LocationUtils.getPersonLocation(patient.getPerson());
if(location!=null) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Same issue with the above line formatting.

@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-2 branch 2 times, most recently from 7d99697 to e461d00 Jul 23, 2018

if (user != null) {
Object userProperty = user.getUserProperty(LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME);
if (userProperty!=null) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can you also put spaces for the line above?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Added Thanks

public class ViewUserLocationFragmentController {

public void controller(FragmentModel model, FragmentConfiguration config) {
Object userUuid = config.getAttribute("userUuid");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can we name it as userId instead of userUuid?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Changes to userId.

"id": "${project.parent.groupId}.${project.parent.artifactId}.createUser.customFragments.userViewLocation",
"extensionPointId": "createUser.manageAccountUserViewFragments",
"extensionParams": {
"label": "locationbasedaccess.location",

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

After replacing the labels, did you test to confirm that all is well?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

No need to mention it here, Since I have added the localize to the fragment. https://github.com/openmrs/openmrs-module-locationbasedaccess/pull/17/files#diff-f7b56ecd636c97d99c293b48b21e54a3R6

@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-2 branch 5 times, most recently from 688a978 to abfc614 Jul 23, 2018
@@ -3,7 +3,7 @@
<div>
<p class="left">
<label>
Please select the location

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Can we drop the "Please select the"? and just use Location?

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

The same applies else where you use please select the location instead of just Location

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

@dkayiwa I have changed that to location.

@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-2 branch from abfc614 to cdfa4ed Jul 24, 2018
@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Jul 24, 2018

Did you also change the extension ids?

@suthagar23
Copy link
Member Author

@suthagar23 suthagar23 commented Jul 24, 2018

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Jul 24, 2018

In the docs, i still see something like createUser.customFragments.userLocation

@@ -0,0 +1,22 @@
[
{
"id": "${project.parent.groupId}.${project.parent.artifactId}.createUser.customFragments.userLocation",

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

I thought we changed from "createUser"?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Just updated @dkayiwa
Sorry, I thought it was just ids specifically for this module.

@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-2 branch from cdfa4ed to 28db6f1 Jul 24, 2018
}
},
{
"id": "${project.parent.groupId}.${project.parent.artifactId}.userAccount.userViewLocation",

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

The above lacks "Property"

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Just added @dkayiwa

@@ -0,0 +1,22 @@
[
{
"id": "${project.parent.groupId}.${project.parent.artifactId}.userAccount.userEditLocation",

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

The above lacks "Property"

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

just added

…stration

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes
@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-2 branch from 28db6f1 to 28379bc Jul 24, 2018
@dkayiwa dkayiwa merged commit 4faaa2f into openmrs:master Jul 24, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
suthagar23 added a commit to suthagar23/openmrs-module-locationbasedaccess that referenced this pull request Jul 27, 2018
…stration (openmrs#17)

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

PR review fixes

LBAC-13 Added implementation to create RefApp location glopal property

LBAC-13 Added implementation to create RefApp location glopal property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants