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

RA-1516 Added support to select the location from the userProperty in the Login Screen #47

Merged
merged 1 commit into from Aug 2, 2018

Conversation

@suthagar23
Copy link
Member

@suthagar23 suthagar23 commented Jul 26, 2018

Description

This PR contains the changes to select the session location from userProperty in the login screen. It will be worked according to these following steps,

  1. It will check for the Global property(GP) called "referenceapplication.locationUserPropertyName".
  2. If the GP has the value, then it will hide the location selector from the login screen
  3. When users try to log in, it will fetch the location from the user property
  4. If the respected user doesn't have the required user property, then it will redirect to the usual login screen(with login selector) to select the location

image

image

Ticket

Ticket : https://issues.openmrs.org/browse/RA-1516


//TODO uncomment this to replace the if clause after it

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

When do you plan to work on the above TODO?

This comment has been minimized.

@suthagar23

suthagar23 Jul 26, 2018
Author Member

Actually this was came from the exiting code. I just moved that whole part. Do you want me to clear this as well?

This comment has been minimized.

@suthagar23

suthagar23 Jul 26, 2018
Author Member

Just removed @dkayiwa

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from 4501a70 to e7e85c1 Jul 26, 2018
@SpringBean("locationService") LocationService locationService,
@SpringBean("appFrameworkService") AppFrameworkService appFrameworkService) {
UiUtils ui,
PageRequest pageRequest,

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

Did you intentionally change this indention?

@@ -110,11 +114,24 @@ public String get(PageModel model,
Context.removeProxyPrivilege(GET_LOCATIONS);
}

model.addAttribute("showSessionLocations", !isLocationUserPropertyAvailable(administrationService));
Object showLocation = pageRequest.getAttribute("showSessionLocations");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

The indention here does not look right

@@ -110,11 +114,24 @@ public String get(PageModel model,
Context.removeProxyPrivilege(GET_LOCATIONS);
}

model.addAttribute("showSessionLocations", !isLocationUserPropertyAvailable(administrationService));

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

Did you intentionally duplicate the model.addAttribute call?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Nope @dkayiwa, It used to set that value always, and if wanted it will update this value by next line

model.addAttribute("lastSessionLocation", lastSessionLocation);

return null;
}

private boolean isLocationUserPropertyAvailable(AdministrationService administrationService) {
String showLocationSelector = administrationService.getGlobalProperty(ReferenceApplicationConstants.LOCATION_USER_PROPERTY_NAME);

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

What if it is available but does not have a value of true?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Actually, I decided to use only one GP for this task, So if that GP contains any value except "false", it will return that value as the userProperty name

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

You have a gp named "referenceapplication.locationUserPropertyName" and then expect a value of true or false? This does not look intuitive because from the look of the gp name, i would expect it to contain the actual name.

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

We decided to use one GP for this, So when evet it is set, we know that it true, if not set or has value as "false" we know it is false.

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

What happens when it has a value of a non existing property?

@@ -187,7 +204,9 @@ private String getRedirectUrl(PageRequest pageRequest) {
public String post(@RequestParam(value = "username", required = false) String username,
@RequestParam(value = "password", required = false) String password,
@RequestParam(value = "sessionLocation", required = false) Integer sessionLocationId,
@SpringBean("locationService") LocationService locationService, UiUtils ui, PageRequest pageRequest,
@SpringBean("locationService") LocationService locationService,
@SpringBean("adminService") AdministrationService administrationService,

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

Did you intentionally change the indention?

return "redirect:" + redirectUrl;
} else {
if (log.isDebugEnabled())
log.debug("Redirect contains 'login.', redirecting to home page");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

Can we avoid changing the indention such that we only get to see meaningful changes?

This comment has been minimized.

@@ -277,7 +316,8 @@ public String post(@RequestParam(value = "username", required = false) String us
//TODO limit login attempts by IP Address

pageRequest.getSession().setAttribute(SESSION_ATTRIBUTE_REDIRECT_URL, redirectUrl);

// Since the user is already authenticated without location, need to logout before redirecting
Context.logout();

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

Why would we log out from a method that is supposed to handle log in?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Since I need to do the context.authenticate() before verifying the location restrictions. So if anything required isn't satisfied, then it will be redirected with the authenticated user. So the user should automatically log out from the system before the redirect.

listItem[i].addEventListener('keyup', function (event){
var keyCode = event.which || event.keyCode;
switch (keyCode) {
case 37: // move left

This comment has been minimized.

@dkayiwa

dkayiwa Jul 26, 2018
Member

Too many indention changes. Can we do or fix the indention using another pull request? It gets hard to see what actually changed.

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

I have done the refactor for those indentions. It occured automatically since I done it through the IntelliJ. Could be better to go through this 😄

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

Just like i said above

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

I have updated all the indentions changes

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

Isn't this another indention?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Extremely sorry @dkayiwa It happened automatically with my IDE. I just made it manually now. Please have a look again

@dkayiwa
Copy link
Member

@dkayiwa dkayiwa commented Jul 26, 2018

We also prefer to attach screenshot on the ticket instead of pull request as per https://wiki.openmrs.org/display/docs/Pull+Request+Tips

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch 19 times, most recently from 2c88ce0 to 42ec855 Jul 27, 2018
@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch 8 times, most recently from 4475672 to ede8d9a Jul 27, 2018
@@ -110,11 +114,24 @@ public String get(PageModel model,
Context.removeProxyPrivilege(GET_LOCATIONS);
}

model.addAttribute("showSessionLocations", !isLocationUserPropertyAvailable(administrationService));
Object showLocation = pageRequest.getAttribute("showSessionLocations");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

I still do not understand why you call model.addAttribute twice instead of once

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Line 117 - Assign the showSessionLocations from the locationUserproperty
Line 119 - If the URL contains the "showSessionLocations" as the paramerter (while redirecting again to the users who haven't location property) then show the location selector (set model attribute as true) (Ignore what ever in the user location property)

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

Can't you do it once?

This comment has been minimized.

@@ -110,11 +114,24 @@ public String get(PageModel model,
Context.removeProxyPrivilege(GET_LOCATIONS);
}

model.addAttribute("showSessionLocations", !isLocationUserPropertyAvailable(administrationService));
Object showLocation = pageRequest.getAttribute("showSessionLocations");
if(showLocation != null && showLocation.toString().equals("true")) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

Are you sure we are expecting this value to be true?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Yes, Initial login URL will be "http://localhost:8080/openmrs/login.htm", and then if the user doesn't have the location userproperty, then it will redirect to "http://localhost:8080/openmrs/referenceapplication/login.page?showSessionLocations=true&"

model.addAttribute("lastSessionLocation", lastSessionLocation);

return null;
}

private boolean isLocationUserPropertyAvailable(AdministrationService administrationService) {
String locationUserPropertyName = administrationService.getGlobalProperty(ReferenceApplicationConstants.LOCATION_USER_PROPERTY_NAME);
if(StringUtils.isNotBlank(locationUserPropertyName) && !locationUserPropertyName.equals("false")) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

I thought we agreed to simply use a valid user property name. Instead instead of true or false

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Yes , Whenever it is set, we know that it is true. If not set, we know it is false.

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from ede8d9a to 8755aef Jul 27, 2018
}
}
catch (ContextAuthenticationException ex) {
if (log.isDebugEnabled())

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Actually, this came from the existing code. Do you want me to remove this as well?

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from 8755aef to 677ceb4 Jul 27, 2018
@@ -23,4 +23,6 @@

public static final long PROCESS_HL7_TASK_INTERVAL = 5L;

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

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

After looking at the other pull request, i have noticed that you expect the user property to be named as locationUuid

In such a case, of what value is this global property?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

Using this Global property(name - referenceapplication.locationUserPropertyName), I want to fetch the userProperty Name(should be value of this GP). Then need to use this userPropertyName to fetch the location from the userProperty(should be value of that userProperty).

So in our module, I'm using "locationUuuid" as the userPropertyName to store the locationUuid. So setting "locationUuid" to this GP.

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

If you know that the property will be named as "locationUuuid", then of what value is the gp?

This comment has been minimized.

@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.

@dkayiwa

dkayiwa Jul 27, 2018
Member

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

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

I think, We have already concluded this discussion 😄

Boolean isLocationUserPropertyAvailable = isLocationUserPropertyAvailable(administrationService);
Object showLocation = pageRequest.getAttribute("showSessionLocations");
if(showLocation != null && showLocation.toString().equals("true")) {
// if the request contains a attribute as showSessionLocations, then ignore isLocationUserPropertyAvailable

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

Confusing how showLocation is true and then you set is location user property available to false?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

if the URL has the parameter as "showSessionLocations" with value True, then we need to ignore the userProperty method(if exist or not). So simply setting isLocationUserPropertyAvailable=false to disable this feature to view the location selector in the login screen. (See the address bar of the browser in the attached images in the PR)

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

who sets the showSessionLocations parameter to true and when?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

When a user doesn't get the location userProperty, then it will redirect to the login page with showSessionLocations=true. So I'm checking this parameter to show/hide the location selector here. See this line - https://github.com/openmrs/openmrs-module-referenceapplication/pull/47/files#diff-bd998e9f6ec90d297ceba7fb14c2fe66R249

if (Context.isAuthenticated() && Context.getUserContext().getAuthenticatedUser() != null) {
String locationUuid = Context.getUserContext().getAuthenticatedUser().getUserProperty(locationUserPropertyName);
if (StringUtils.isNotBlank(locationUuid)) {
sessionLocation = locationService.getLocationByUuid(locationUuid);

This comment has been minimized.

@dkayiwa

dkayiwa Jul 27, 2018
Member

What happens when the uuid is for a non existing location?

This comment has been minimized.

@suthagar23

suthagar23 Jul 27, 2018
Author Member

It will return the null object to the sessionLocation. So it might cause some issues to the next line.
Just added a fix to this problem. Please look at this line again @dkayiwa

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from 677ceb4 to 1a76d98 Jul 27, 2018
sessionLocation = locationService.getLocationByUuid(locationUuid);
}
if (sessionLocation != null) {
sessionLocationId = sessionLocation.getLocationId();

This comment has been minimized.

@dkayiwa

dkayiwa Jul 30, 2018
Member

Can you check the indention here?

This comment has been minimized.

@suthagar23

suthagar23 Jul 30, 2018
Author Member

Fixed @dkayiwa

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from 1a76d98 to 45ea24c Jul 30, 2018
sessionLocationId = sessionLocation.getLocationId();
}
else {
pageRequest.getSession().setAttribute(ReferenceApplicationWebConstants.SESSION_ATTRIBUTE_ERROR_MESSAGE,

This comment has been minimized.

@dkayiwa

dkayiwa Jul 30, 2018
Member

Can you look into the indention here?

This comment has been minimized.

@suthagar23

suthagar23 Aug 2, 2018
Author Member

Fixed

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from 45ea24c to 341d0e7 Aug 1, 2018
if (sessionLocation != null) {
sessionLocationId = sessionLocation.getLocationId();
}
else {

This comment has been minimized.

@dkayiwa

dkayiwa Aug 1, 2018
Member

The formatting or braces around here do not look correct.

This comment has been minimized.

@suthagar23

suthagar23 Aug 2, 2018
Author Member

Fixed

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch 3 times, most recently from a3965e8 to d3a5ae1 Aug 2, 2018
… the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

RA-1516 Added support to select the location from the userProperty in the Login Screen

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

Indention changes

LBAC-13 Added implementation to create RefApp location glopal property

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes

Added changes
@suthagar23 suthagar23 force-pushed the suthagar23:RA-1516 branch from d3a5ae1 to f1ee7da Aug 2, 2018
@dkayiwa dkayiwa merged commit 8d605a5 into openmrs:master Aug 2, 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants