-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
TRUNK-5367: Create URL global property on OpenMRS core #2657
Conversation
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just login.html?
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just login.html?
@dkayiwa resolved. |
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use of ../../
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value should be login.htm
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use of ../../
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default should be login.htm
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to write login.htm in two places? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, one of them is just a description of the key pair value of the global properties.
@@ -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, "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment did not mean removing the default value. Am just guiding you to something else. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kindly elaborate..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean removing the global declaration and combining it with the addition of props.add(new GlobalProperty (...... Global property declaration + value inside here.. ))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😊
92a47af
to
bf6d24e
Compare
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you take a look at the expected parameters for the GlobalProperty constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea it expects a property, a value, and a description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tell me the actual value that you are passing in for each of those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean value for each of the parameters.
- property, 2) value, 3) description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- login.htm, 2) login.htm, 3) "Responsible for defining the Authentication URL."
Mhh something seems wrong on the property and the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly what i wanted you to see for yourself. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks.
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you take a look at this class to see the convention used for naming other global property constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had not, now i have
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at this for the convention of global property names: https://demo.openmrs.org/openmrs/admin/maintenance/globalProps.form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that page https://demo.openmrs.org/openmrs/admin/maintenance/globalProps.form have anything like GO or GLOBAL_PROPERTY prefix? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above worked.
in the question above, I was seeking clarification regarding how GLOBAL PROPERTY VARIABLES they are labeled in the codebase
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some javadoc to these two constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this okay now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which ones have you documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOGIN_URL AND GP_LOGIN_URL
62f2318
to
f92a786
Compare
* | ||
* @see #LOGIN_URL | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the above empty line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
/** | ||
* Global property string value which is the file name provided | ||
* after successful routing to the default authentication page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the above description correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrased..
cc089a9
to
64384a0
Compare
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description is not in line with how you have described GP_LOGIN_URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this closer home?
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved?
This reverts commit df3c95e.
This reverts commit df3c95e.
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 andadded 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