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-1513 Added support to allow the extensions to add the custom fragments to the Manage Account dashboard #44

Merged
merged 1 commit into from Jul 24, 2018

Conversation

@suthagar23
Copy link
Member

@suthagar23 suthagar23 commented Jul 20, 2018

Description

Along with this PR,

  • Add New Account/Edit account dashboards can allow the fragments through the extensions
  • It can allow the different fragments to Add/Edit Information and View information
  • It can provide supports to save the person attributes (if the fragment field type is personAttribute) automatically along with the person registration/ person update.
  • It can provide supports to save the user properties (if the fragment field type is userProperty) automatically along with the user registration/ user update.

Ticket Information

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

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from 2e0916a to b25fc14 Jul 20, 2018
@suthagar23 suthagar23 changed the title RA-1513 Added support to allow the extensions to add the custom fragments to the dashboard RA-1513 Added support to allow the extensions to add the custom fragments to the Manage Account dashboard Jul 20, 2018
@suthagar23 suthagar23 requested a review from wluyima Jul 20, 2018
if (person != null) {
PersonAttribute attr = person.getAttribute(Context.getPersonService().getPersonAttributeTypeByUuid(
attributeTypeUuid));
if (attr != null) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

What value does checking for not null add?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

No need it here, removed

@@ -13,6 +20,7 @@ angular.module("adminui.personDetails", ["personService"])
$scope.person.familyName = familyName;
$scope.person.givenName = givenName;
$scope.genders = {M: male, F: female};
$scope.personAttributeInfoMap = personAttributeInfoMap;

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

What value does "Info" add to the name?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Changed

}
}
}

This comment has been minimized.

@dkayiwa

dkayiwa Jul 22, 2018
Member

Can we do some bit of uniformity with our indention?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Sorry, I didn't get you properly

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

The indention is not the same. Can you look at it again?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Do you mention the implementation after this line which points to person attribute? I used the existing patient registration implementation to build this one. Any brief comments to make this better?

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can you look at lines 138 and 156?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

You mentioned about the names at these lines? "createUser.manageAccountUserFragments" and "createUser.manageAccountPersonFragments"

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

At what vertical column numbers do they start? In other words, do they have the same indention?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

Just got it, and fixed it as well. Please see the updated code

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch 2 times, most recently from d28ab9b to 1703c78 Jul 23, 2018
String[] parameterValues = parameterMap.get(formFiledName);
if (parameterValues != null && parameterValues.length > 0) {
if (parameterValues.length > 1) {
log.warn("Multiple values for a single person attribute type not supported, ignoring extra values");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Shouldn't you throw an exception for the above?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

if we sent the exception, then it will break the registration workflow. It will happen when the user configured the dashboard with duplicate filed ids. In the patient registration also, We followed this way.

Object type = ext.getExtensionParams().get("type");
Object personAttributeTypeUuid = ext.getExtensionParams().get("uuid");
Person person = account.getPerson();
if (person!=null && type!=null && personAttributeTypeUuid!=null &&

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can we put some spaces for != null

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

added

@@ -10,6 +10,11 @@
%>

<% if(!createAccount) { %>
<script type="text/javascript">
//This function is in personDetails.js

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Do we need the above comment?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

removed @dkayiwa

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from 1703c78 to 6190223 Jul 23, 2018
title : ui.message(sect.extensionParams.title),
label : ui.message(sect.extensionParams.label),
initialValue : ui.message(account.getPersonAttribute(sect.extensionParams.uuid) != null ?
account.getPersonAttribute(sect.extensionParams.uuid).value : '')

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can we indent the above line correctly?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

That is the continue part of the above line. Does it contain wrong indent?

var personToSave = {
uuid: $scope.person.personUuid,
gender: $scope.person.gender,
names: [{
uuid: $scope.person.personNameUuid,
familyName: $scope.person.familyName,
givenName: $scope.person.givenName}]
givenName: $scope.person.givenName}],
attributes: attributesSet

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can you indent the above correctly?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

changed

personId : account.person.personId,
personUuid : account.person.uuid,
initialValue : ui.message(account.getPersonAttribute(sect.extensionParams.uuid) != null ?
account.getPersonAttribute(sect.extensionParams.uuid).value : '')

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can you indent the above correctly?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

That is the continue part of the above line. Does it contain wrong indent?

@@ -138,6 +138,15 @@ angular.module("adminui.userDetails", ["userService", "ngDialog", "adminui.shoul
if(modelUser.userProperties.forcePassword){
uProperties.forcePassword = "true";
}
angular.forEach(modelUser.userProperties, function(value, key) {
if(key != "forcePassword") {
var domElement = document.getElementById(key+userUuid);

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can we put spaces around the + sign?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

changed

@@ -128,6 +128,16 @@
ng-model="uuidUserMap['${userUuid}'].userProperties.forcePassword" />${ ui.message("adminui.account.user.forcePasswordChange") }
</p>

<% customUserFragments.each { sect -> %>
${ ui.includeFragment(sect.extensionParams.provider, sect.extensionParams.fragment, [
formFieldName : sect.extensionParams.userPropertyName+userUuid,

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can we put spaces around the + sign?

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

changed

HttpServletRequest request, UiUtils uu) throws IOException {

Errors errors = new BeanPropertyBindingResult(account, "account");

List<Extension> customUserFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountUserFragments");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Do you have any reason for making createUser.manageAccountUserFragments plural?
The same applies for the rest downwards.

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

No need, better to change it to "createUser.manageAccountUserFragment"

log.warn("Multiple userProperty for a single user type not supported, ignoring extra values");
}
String parameterValue = parameterValues[0];
if (userPropertyName !=null && parameterValue != null) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Can we also put space around the above !=

This comment has been minimized.

@suthagar23

suthagar23 Jul 23, 2018
Author Member

changed

@@ -203,7 +254,21 @@ public void setModelAttributes(PageModel model, Account account, OtherAccountDat
model.addAttribute("allowedLocales", administrationService.getAllowedLocales());
List<ProviderRole> providerRoles = providerManagementService.getAllProviderRoles(false);
model.addAttribute("providerRoles", providerRoles);


List<Extension> customPersonFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountPersonFragments");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Since the other one has "View" shouldn't this have "Edit"?

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from 6190223 to a631a90 Jul 23, 2018


List<Extension> customPersonFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountPersonFragment");
Collections.sort(customPersonFragments);

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Since the other one one has "View", shouldn't this have "Edit"?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

replaced with "Edit"

@@ -128,6 +128,16 @@
ng-model="uuidUserMap['${userUuid}'].userProperties.forcePassword" />${ ui.message("adminui.account.user.forcePasswordChange") }
</p>

<% customUserFragments.each { sect -> %>

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

Since the other one one has "View", shouldn't this have "Edit"?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

replaced with "Edit"

@@ -38,6 +42,19 @@
<th valign="top">${ui.message("Person.gender")}</th>
<td valign="top">{{genders[person.gender]}}</td>
</tr>

<% customPersonViewFragments.each { sect ->

This comment has been minimized.

@dkayiwa

dkayiwa Jul 23, 2018
Member

What does "sect" mean?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Updated with "fragment"

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from a631a90 to 29246e6 Jul 24, 2018
HttpServletRequest request, UiUtils uu) throws IOException {

Errors errors = new BeanPropertyBindingResult(account, "account");

List<Extension> customUserFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountUserFragment");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

I thought this would include "UserProperty" in its name?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Changed to "customUserPropertyFragments"

}
}

List<Extension> customPersonFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountPersonFragment");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

I thought this would include "PersonAttribute" in its name?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Changed to "customPersonAttributeFragments"

@@ -203,7 +254,21 @@ public void setModelAttributes(PageModel model, Account account, OtherAccountDat
model.addAttribute("allowedLocales", administrationService.getAllowedLocales());
List<ProviderRole> providerRoles = providerManagementService.getAllProviderRoles(false);
model.addAttribute("providerRoles", providerRoles);


List<Extension> customPersonEditFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountPersonFragment");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

If this is dealing with strictly person attributes, then it should include "PersonAttribute" in its name.

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Changed

Collections.sort(customPersonViewFragments);
model.addAttribute("customPersonViewFragments", customPersonViewFragments);

List<Extension> customUserEditFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountUserFragment");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

If this is dealing with strictly user properties, then it should include "UserProperty" in its name.

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

changed

@@ -250,6 +315,12 @@ public void setModelAttributes(PageModel model, Account account, OtherAccountDat
force = (user.getUserId() == null) ? true : account.isSupposedToChangePassword(user);
}
userProperties.put(OpenmrsConstants.USER_PROPERTY_CHANGE_PASSWORD, force);

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Same here as above.

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

I think you have mentioned the next line. Changed



so = new SimpleObject();
for(Extension ext : customPersonEditFragments) {

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Same here as above.

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

changed

@@ -38,6 +42,19 @@
<th valign="top">${ui.message("Person.gender")}</th>
<td valign="top">{{genders[person.gender]}}</td>
</tr>

<% customPersonViewFragments.each { fragment ->

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Same here as above.

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

changed

@@ -97,6 +114,20 @@
${ui.message("adminui.field.required")}
</span>
</span>

<% customPersonEditFragments.each { fragment ->

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Same here as above

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

changed

@@ -128,6 +128,16 @@
ng-model="uuidUserMap['${userUuid}'].userProperties.forcePassword" />${ ui.message("adminui.account.user.forcePasswordChange") }
</p>

<% customUserEditFragments.each { fragment -> %>

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Same here as above

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

changed

@@ -30,6 +31,18 @@
{{yesOrNo[uuidUserMap['${uuid}'].userProperties.forcePassword]}}
</td>
</tr>

<% customUserViewFragments.each { fragment -> %>

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Same here as above

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

changed

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from 29246e6 to c7933f9 Jul 24, 2018
HttpServletRequest request, UiUtils uu) throws IOException {

Errors errors = new BeanPropertyBindingResult(account, "account");

List<Extension> customUserPropertyFragments = appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountUserFragment");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

The extension names also need to reflect these changes.

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Changed @dkayiwa

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from c7933f9 to e63eb92 Jul 24, 2018


List<Extension> customPersonAttributeEditFragments =
appFrameworkService.getExtensionsForCurrentUser("createUser.manageAccountPersonAttributeFragment");

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Shouldn't the extension name include "Edit"?

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

@dkayiwa Just changed to "createUser.manageAccountPersonAttributeEditFragment"

@suthagar23 suthagar23 force-pushed the suthagar23:RA-1513 branch from e63eb92 to ef6fca2 Jul 24, 2018
}

/**
* Gets the person attribute for the specified person for the getPersonAttributeTypeByUuid

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Does the javadoc have to talk about implementation details? (The actual API method being called)

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Removed

* Gets the person attribute for the specified person for the getPersonAttributeTypeByUuid
* that matches the specified uuid
*
* @return the attribute

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Can you document the above return?

HttpServletRequest request, UiUtils uu) throws IOException {

Errors errors = new BeanPropertyBindingResult(account, "account");

List<Extension> customUserPropertyEditFragments =

This comment has been minimized.

@dkayiwa

dkayiwa Jul 24, 2018
Member

Since some times you are not creating a user, but just updating their details, can we use a convention like? userAccount.userPropertyEditFragment

This comment has been minimized.

@suthagar23

suthagar23 Jul 24, 2018
Author Member

Changed @dkayiwa

…ents to the dashboard

Minor fix

Minor fix

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:RA-1513 branch from ef6fca2 to c004c11 Jul 24, 2018
@dkayiwa dkayiwa merged commit e3832ab into openmrs:master Jul 24, 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