Skip to content

/ openmrs-module-locationbasedaccess

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

Jump to bottom

LBAC-13 Added implementation to create RefApp location glopal property #19

Merged
merged 1 commit into from Jul 27, 2018
Merged

LBAC-13 Added implementation to create RefApp location glopal property #19

merged 1 commit into from Jul 27, 2018

Conversation

@suthagar23
Copy link
Member

@suthagar23 suthagar23 commented Jul 26, 2018

Description

This PR contains the implementation to create the refapp location global propery while starting the module.

Ticket Information

Ticekt : https://issues.openmrs.org/browse/LBAC-13
@dkayiwa
dkayiwa reviewed Jul 26, 2018
View changes
api/src/main/java/org/openmrs/module/locationbasedaccess/LocationBasedAccessActivator.java Outdated
@@ -45,6 +47,16 @@ public void willStart() {
* @see ModuleActivator#started()
*/
public void started() {
String refAppLocationGlobalProperty = Context.getAdministrationService().getGlobalProperty(LocationBasedAccessConstants.REF_APP_LOCATION_USER_PROPERTY_NAME);
if(StringUtils.isNotBlank(refAppLocationGlobalProperty) && refAppLocationGlobalProperty != LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME) {

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 26, 2018
Member

Does this give a user the flexibility of turning it off with a different value?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

Yes, If the value exists in the system, then it will exit from creating the new global property.
@dkayiwa
dkayiwa reviewed Jul 26, 2018
View changes
api/src/main/java/org/openmrs/module/locationbasedaccess/LocationBasedAccessActivator.java Outdated
@@ -45,6 +47,16 @@ public void willStart() {
* @see ModuleActivator#started()
*/
public void started() {
String refAppLocationGlobalProperty = Context.getAdministrationService().getGlobalProperty(LocationBasedAccessConstants.REF_APP_LOCATION_USER_PROPERTY_NAME);
if(StringUtils.isNotBlank(refAppLocationGlobalProperty) && refAppLocationGlobalProperty != LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME) {

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 26, 2018
Member

String comparison is not usually done like that.

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

Added fix @dkayiwa
@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-13 branch from 4af6252 to b5507b5 Jul 27, 2018
@dkayiwa
dkayiwa reviewed Jul 27, 2018
View changes
api/src/main/java/org/openmrs/module/locationbasedaccess/LocationBasedAccessActivator.java Outdated
@@ -45,6 +47,19 @@ public void willStart() {
* @see ModuleActivator#started()
*/
public void started() {
String refAppLocationGlobalProperty = Context.getAdministrationService().getGlobalProperty(LocationBasedAccessConstants.REF_APP_LOCATION_USER_PROPERTY_NAME);

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

What does ref app global property mean?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

better to change as "locationUserPropertyName". Do you agree?

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

That would be better.
@dkayiwa
dkayiwa reviewed Jul 27, 2018
View changes
api/src/main/java/org/openmrs/module/locationbasedaccess/LocationBasedAccessActivator.java Outdated
log.debug("RefApp Location global property created with value - " + LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME);
}
}
else if(StringUtils.isNotBlank(refAppLocationGlobalProperty) && !refAppLocationGlobalProperty.equals(LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME)) {

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

I thought this was just a flag that is set to either true or false?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

Then we need two Global properties for this implementation.

  1. One for True or False (to show locations or not)
  2. To get the location user property name
    Do want me to make this by two GPs?

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

I think one instead of two would make it less work to configure. In that case it will be a property pointing to the user property name. Whenever it is set, we know that it is true. If not set, we know it is false.

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

In addition to this, I thought to have when evet it is set, we know that it true, if not set or has value as "false" we know it is false. Is it alright?

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

What happens when it points to wrong property name?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

that might be a problem. So better to remove this and, follow Whenever it is set, we know that it is true. If not set, we know it is false.

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

In this line, it is checking for the exisiting value with the actual value to show the WARN message.
@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-13 branch 2 times, most recently from 1b7e0b7 to 49e837c Jul 27, 2018
Suthagar23
LBAC-13 Added implementation to create RefApp location glopal property
Loading status checks…
be66611 
LBAC-13 Added implementation to create RefApp location glopal property
@suthagar23 suthagar23 force-pushed the suthagar23:LBAC-13 branch from 49e837c to be66611 Jul 27, 2018
@dkayiwa
dkayiwa reviewed Jul 27, 2018
View changes
api/src/main/java/org/openmrs/module/locationbasedaccess/LocationBasedAccessConstants.java
@@ -14,4 +14,6 @@

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

public static final String REF_APP_LOCATION_USER_PROPERTY_NAME = "referenceapplication.locationUserPropertyName";

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

Of what value is this GP when you expect the user property to be named as "locationUuid"?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

exactly "locationUuid" as the GP value

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

We do not do global properties for fixed values that we know. Otherwise, of what value would the gp be?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

So can we move the locationUserProperty to a global property name as "locationbasedaccess.locationUserPropertyName"? So the userProperty name also will be fetched from the GP, and use that same value to this RefApp GP as well?

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

What do you think? What are the pros and cons of either side?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

No need to create new one, Will reopen this : https://issues.openmrs.org/browse/LBAC-2

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

We decided to drop this idea and go with it for now. So are there any more comments?

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

So what are you using the gp for?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

We need this to connect our module with RefApp module without any direct coupling. Since both need this userPropertyName. So we can connect them through this GP.
Otherwise we need to hardcode this value in both module.

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

Got you! 😊
@dkayiwa
dkayiwa reviewed Jul 27, 2018
View changes
api/src/main/java/org/openmrs/module/locationbasedaccess/LocationBasedAccessActivator.java
@@ -45,6 +47,18 @@ public void willStart() {
* @see ModuleActivator#started()
*/
public void started() {
String locationUserPropertyName = Context.getAdministrationService().getGlobalProperty(LocationBasedAccessConstants.REF_APP_LOCATION_USER_PROPERTY_NAME);
if(StringUtils.isBlank(locationUserPropertyName)) {
Context.getAdministrationService().setGlobalProperty(LocationBasedAccessConstants.REF_APP_LOCATION_USER_PROPERTY_NAME,

This comment has been minimized.

Sign in to view
@dkayiwa

dkayiwa Jul 27, 2018
Member

If you expect the value to be LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME, then why are you doing a gp for it?

This comment has been minimized.

Sign in to view
@suthagar23

suthagar23 Jul 27, 2018
Author Member

The reason was, in our module, we can use this LocationBasedAccessConstants.LOCATION_USER_PROPERTY_NAME.
But in the RefApp module, it should be decoupled to use another property names for different usages. So I used the RefApp GP to track that value, and for only our module I used this.
@dkayiwa dkayiwa merged commit ecfa6c7 into openmrs:master Jul 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkayiwa dkayiwa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@suthagar23 @dkayiwa