TRUNK-5367: Create URL global property on OpenMRS core #2657

mwaz · 2018-04-19T10:07:15Z

Description of what I changed

I added a global property LOGIN URL to hold the default login URL and be accessible publicly.

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5367

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

coveralls · 2018-04-19T10:18:51Z

Coverage increased (+0.1%) to 58.961% when pulling 1bf107d on mwaz:TRUNK-5367 into 0e49413 on openmrs:master.

dkayiwa · 2018-04-19T14:39:57Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1081,6 +1081,8 @@ public static String getOpenmrsProperty(String property) {

 props
 .add(new GlobalProperty(GP_DRUG_ORDER_DRUG_OTHER, "", "Specifies the uuid of the concept which represents drug other non coded"));
+ props.add(new GlobalProperty(LOGIN_URL, "../../login.htm",


Why not just login.html?

dkayiwa · 2018-04-19T14:40:10Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1235,6 +1237,8 @@ public static String getOpenmrsProperty(String property) {

 public static final String LOG_LEVEL_FATAL = "fatal";

+ public static final String LOGIN_URL = "../../login.htm";


Why not just login.html?

mwaz · 2018-04-19T17:36:20Z

@dkayiwa resolved.

dkayiwa · 2018-04-19T19:35:42Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1081,6 +1081,8 @@ public static String getOpenmrsProperty(String property) {

 props
 .add(new GlobalProperty(GP_DRUG_ORDER_DRUG_OTHER, "", "Specifies the uuid of the concept which represents drug other non coded"));
+ props.add(new GlobalProperty(LOGIN_URL, "../../login.html",


What is the use of ../../

The default value should be login.htm

dkayiwa · 2018-04-19T19:35:57Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1235,6 +1237,8 @@ public static String getOpenmrsProperty(String property) {

 public static final String LOG_LEVEL_FATAL = "fatal";

+ public static final String LOGIN_URL = "../../login.html";


What is the use of ../../

dkayiwa · 2018-04-20T08:25:51Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1081,6 +1081,8 @@ public static String getOpenmrsProperty(String property) {

 props
 .add(new GlobalProperty(GP_DRUG_ORDER_DRUG_OTHER, "", "Specifies the uuid of the concept which represents drug other non coded"));
+ props.add(new GlobalProperty(LOGIN_URL, "login.html",


The default should be login.htm

dkayiwa · 2018-04-20T09:15:48Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1081,6 +1081,8 @@ public static String getOpenmrsProperty(String property) {

 props
 .add(new GlobalProperty(GP_DRUG_ORDER_DRUG_OTHER, "", "Specifies the uuid of the concept which represents drug other non coded"));
+ props.add(new GlobalProperty(LOGIN_URL, "login.htm",


Do you need to write login.htm in two places? 😊

Nope, one of them is just a description of the key pair value of the global properties.

dkayiwa · 2018-04-20T09:28:21Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1081,6 +1081,8 @@ public static String getOpenmrsProperty(String property) {

 props
 .add(new GlobalProperty(GP_DRUG_ORDER_DRUG_OTHER, "", "Specifies the uuid of the concept which represents drug other non coded"));
+ props.add(new GlobalProperty(LOGIN_URL, "",


My comment did not mean removing the default value. Am just guiding you to something else. 😊

kindly elaborate..

do you mean removing the global declaration and combining it with the addition of props.add(new GlobalProperty (...... Global property declaration + value inside here.. ))

First of all, get it back to what it was. Then show my original comment to your fellow apprentices and ask them what i could be driving at. 😊

dkayiwa · 2018-04-20T12:59:01Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1081,6 +1081,8 @@ public static String getOpenmrsProperty(String property) {

 props
 .add(new GlobalProperty(GP_DRUG_ORDER_DRUG_OTHER, "", "Specifies the uuid of the concept which represents drug other non coded"));
+ props.add(new GlobalProperty(LOGIN_URL, LOGIN_URL,


Did you take a look at the expected parameters for the GlobalProperty constructor?

yea it expects a property, a value, and a description

Can you tell me the actual value that you are passing in for each of those?

in the GlobalProperty(1, 2, 3)
The first property is login.htm, the value of the property is login.htm and the description is "Responsible for defining the Authentication URL."

I mean value for each of the parameters.

property, 2) value, 3) description

login.htm, 2) login.htm, 3) "Responsible for defining the Authentication URL."

Mhh something seems wrong on the property and the value.

That is exactly what i wanted you to see for yourself. 😊

dkayiwa · 2018-04-20T15:01:42Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1235,6 +1237,10 @@ public static String getOpenmrsProperty(String property) {

 public static final String LOG_LEVEL_FATAL = "fatal";

+ public static final String LOGIN_URL = "login.htm";
+
+ public static final String LOGIN_URL_PROPERTY = "LOGIN_URL";


Did you take a look at this class to see the convention used for naming other global property constants?

had not, now i have

dkayiwa · 2018-04-21T19:23:22Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1235,6 +1237,10 @@ public static String getOpenmrsProperty(String property) {

 public static final String LOG_LEVEL_FATAL = "fatal";

+ public static final String LOGIN_URL = "login.htm";
+
+ public static final String GP_LOGIN_URL = "LOGIN_URL";


Take a look at this for the convention of global property names: https://demo.openmrs.org/openmrs/admin/maintenance/globalProps.form

there are two types of global property naming systems, how does one differentiate between using the GP or GLOBAL_PROPERTY prefix or just using the name such as "allergy.concept.reactions"

Does that page https://demo.openmrs.org/openmrs/admin/maintenance/globalProps.form have anything like GO or GLOBAL_PROPERTY prefix? 😊

the above worked.

in the question above, I was seeking clarification regarding how GLOBAL PROPERTY VARIABLES they are labeled in the codebase

dkayiwa · 2018-04-22T20:44:00Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1235,6 +1237,10 @@ public static String getOpenmrsProperty(String property) {

 public static final String LOG_LEVEL_FATAL = "fatal";

+ public static final String LOGIN_URL = "login.htm";


Can you add some javadoc to these two constants?

is this okay now?

Which ones have you documented?

LOGIN_URL AND GP_LOGIN_URL

dkayiwa · 2018-04-23T11:11:09Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

+ *
+ * @see #LOGIN_URL
+ */
+


Can you remove the above empty line?

dkayiwa · 2018-04-23T11:11:40Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

+
+ /**
+ * Global property string value which is the file name provided 
+ * after successful routing to the default authentication page.


Is the above description correct?

rephrased..

dkayiwa · 2018-04-23T11:22:09Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1234,6 +1236,22 @@ public static String getOpenmrsProperty(String property) {
 public static final String LOG_LEVEL_ERROR = "error";

 public static final String LOG_LEVEL_FATAL = "fatal";
+
+ /**
+ * Global property value which defines the default file 


The description is not in line with how you have described GP_LOGIN_URL

is this closer home?

dkayiwa · 2018-04-23T11:46:29Z

api/src/main/java/org/openmrs/util/OpenmrsConstants.java

@@ -1234,6 +1236,22 @@ public static String getOpenmrsProperty(String property) {
 public static final String LOG_LEVEL_ERROR = "error";

 public static final String LOG_LEVEL_FATAL = "fatal";
+
+ /**
+ * Global property value which defines the default authentication


GP_LOGIN_URL is well described.
But the description for LOGIN_URL is still not matching what GP_LOGIN_URL talks about.
Can you take a look an existing properties descriptions for an example?
Feel free to ask your team mates for help.

This reverts commit df3c95e.

dkayiwa reviewed Apr 19, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from bab68be to ad18d40 Compare April 19, 2018 17:33

dkayiwa reviewed Apr 19, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from ad18d40 to 0466a6a Compare April 19, 2018 19:57

dkayiwa reviewed Apr 20, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from 0466a6a to 7a809c1 Compare April 20, 2018 08:49

dkayiwa reviewed Apr 20, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from 7a809c1 to 49cdcc8 Compare April 20, 2018 09:26

dkayiwa reviewed Apr 20, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch 2 times, most recently from 92a47af to bf6d24e Compare April 20, 2018 12:52

dkayiwa reviewed Apr 20, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from bf6d24e to 9f49765 Compare April 20, 2018 14:51

dkayiwa reviewed Apr 20, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from 9f49765 to 8f3d8e6 Compare April 20, 2018 19:00

dkayiwa reviewed Apr 21, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from 8f3d8e6 to 3c4ff2e Compare April 22, 2018 20:32

dkayiwa reviewed Apr 22, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch 2 times, most recently from 62f2318 to f92a786 Compare April 23, 2018 11:08

dkayiwa reviewed Apr 23, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch 3 times, most recently from cc089a9 to 64384a0 Compare April 23, 2018 11:17

dkayiwa reviewed Apr 23, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from 64384a0 to 40f7d6f Compare April 23, 2018 11:32

dkayiwa reviewed Apr 23, 2018

View reviewed changes

mwaz force-pushed the TRUNK-5367 branch from 40f7d6f to 75ddc95 Compare April 23, 2018 13:50

TRUNK-5367 Create URL global property

1bf107d

mwaz force-pushed the TRUNK-5367 branch from 75ddc95 to 1bf107d Compare April 23, 2018 15:02

dkayiwa merged commit aee5246 into openmrs:master Apr 23, 2018

jtatia pushed a commit to jtatia/openmrs-core that referenced this pull request Aug 20, 2018

TRUNK-5367 Create URL global property (openmrs#2657)

05b3733

JyothsnaAshok pushed a commit to JyothsnaAshok/openmrs-core that referenced this pull request Oct 23, 2018

TRUNK-5367 Create URL global property (openmrs#2657)

df3c95e

JyothsnaAshok added a commit to JyothsnaAshok/openmrs-core that referenced this pull request Oct 23, 2018

Revert "TRUNK-5367 Create URL global property (openmrs#2657)"

cb69969

This reverts commit df3c95e.

JyothsnaAshok added a commit to JyothsnaAshok/openmrs-core that referenced this pull request Oct 23, 2018

Revert "TRUNK-5367 Create URL global property (openmrs#2657)"

971a1b6

This reverts commit df3c95e.

		@@ -1235,6 +1237,8 @@ public static String getOpenmrsProperty(String property) {

		public static final String LOG_LEVEL_FATAL = "fatal";

		public static final String LOGIN_URL = "../../login.htm";

		@@ -1235,6 +1237,10 @@ public static String getOpenmrsProperty(String property) {

		public static final String LOG_LEVEL_FATAL = "fatal";

		public static final String LOGIN_URL = "login.htm";

TRUNK-5367: Create URL global property on OpenMRS core #2657

TRUNK-5367: Create URL global property on OpenMRS core #2657

Conversation

mwaz commented Apr 19, 2018 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

coveralls commented Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

dkayiwa Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

dkayiwa Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

mwaz Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaz commented Apr 19, 2018 •

edited

Loading

coveralls commented Apr 19, 2018 •

edited

Loading

mwaz Apr 20, 2018 •

edited

Loading

mwaz Apr 20, 2018 •

edited

Loading

dkayiwa Apr 20, 2018 •

edited

Loading

mwaz Apr 22, 2018 •

edited

Loading

dkayiwa Apr 22, 2018 •

edited

Loading

mwaz Apr 22, 2018 •

edited

Loading

mwaz Apr 23, 2018 •

edited

Loading