Skip to content

RA-1928: LoginPageController to handle URLs with one "/" only.#91

Merged
dkayiwa merged 9 commits intoopenmrs:masterfrom
icrc-jfigueiredo:RA-1928
Jun 28, 2021
Merged

RA-1928: LoginPageController to handle URLs with one "/" only.#91
dkayiwa merged 9 commits intoopenmrs:masterfrom
icrc-jfigueiredo:RA-1928

Conversation

@icrc-jfigueiredo
Copy link
Copy Markdown
Contributor

@icrc-jfigueiredo icrc-jfigueiredo commented May 31, 2021

RA-1928 If only contains one "/" character, it will use the entire urlPath for the urlContextPath
Issue: https://issues.openmrs.org/browse/RA-1928

@HerbertYiga
Copy link
Copy Markdown
Contributor

thanks @icrc-jfigueiredo you can also add the ticket id here

@mks-d mks-d changed the title RA-1928 If only contains one "/" on urlPath, use the entire urlPath. RA-1928: LoginPageController to handle URLs with one "/" only. Jun 22, 2021
@mks-d mks-d requested a review from dkayiwa June 22, 2021 09:21
when(Context.isAuthenticated()).thenReturn(true);
when(Context.getUserContext()).thenReturn(mock(UserContext.class));

String redirectFullUrl = "http://openmrs.org/openmrs";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these variables or constants?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constants, I update according to the constants convention.

*/
@Test
@Verifies(value = "should redirect user to home when user is authenticated if ? redirectUrl is for openrms, even if only have '/openrms'", method = "get(PageModel,UiUtils,PageRequest)")
public void get_shouldRedirectUserToHomeIfAuthenticatedAndPathForOpenmrsWasGivenOnUrl() throws Exception {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also improve the test method name?

Copy link
Copy Markdown
Contributor Author

@icrc-jfigueiredo icrc-jfigueiredo Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @dkayiwa , absolutely, do you have some suggestions? maybe something like:
get_shouldRedirectUserToHomeIfAuthenticatedAndPathUrlAreInOpenmrsScope
get_shouldRedirectUserToHomeIfAuthenticatedAndUrlsForOpenmrs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pick what you see best.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkayiwa i renamed it to get_shouldRedirectUserToHomeIfAuthenticatedAndURLPathAreInOpenmrsScope

@icrc-jfigueiredo icrc-jfigueiredo requested a review from dkayiwa June 28, 2021 08:18
@dkayiwa dkayiwa merged commit dc9b76e into openmrs:master Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants