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-552:Adding the View Logged in Users functionality to core #3706

Closed
wants to merge 4 commits into from

Conversation

HerbertYiga
Copy link
Contributor

ticket Id:https://issues.openmrs.org/browse/RA-552

Adding the View Logged in Users functionality to core

@HerbertYiga HerbertYiga changed the title Adding the View Logged in Users functionality to core RA-552:Adding the View Logged in Users functionality to core Mar 1, 2021
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import javax.servlet.http.HttpSession;
Copy link
Contributor Author

@HerbertYiga HerbertYiga Mar 1, 2021

Choose a reason for hiding this comment

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

Hi @brandones so am adding the view users functionality in core as suggested by @djazayeri here https://talk.openmrs.org/t/view-logged-in-users-is-empty/19348/3, just a quick question, why not have HttpSession and ServletContext implemented at core?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I would not be the person to answer that question. @dkayiwa or @mseaton , maybe?

Copy link
Member

Choose a reason for hiding this comment

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

@HerbertYiga in core, they are not in the api but web module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkayiwa does this mean it will be a nice idea to add this functionality under the web module ?

Copy link
Member

Choose a reason for hiding this comment

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

True dat!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkayiwa awesome, let me go on and do that

Copy link
Member

Choose a reason for hiding this comment

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

Remember to test and confirm that it all works.

@sherrif10
Copy link
Member

Thanks @HerbertYiga feel free to check out my comment on the ticket, however i would also think this should be fixed in legacyui

@HerbertYiga
Copy link
Contributor Author

thanks @sharif this exists in ui and a suggestion came in to have it under core

@sherrif10
Copy link
Member

Do you mind pointing me to that thread talking about core please thanks

@HerbertYiga
Copy link
Contributor Author

@dkayiwa
Copy link
Member

dkayiwa commented Mar 2, 2021

@HerbertYiga did you see the travis build failure?

@HerbertYiga
Copy link
Contributor Author

@dkayiwa yes i saw the Travis build failure, this breaks because i didn't include the HttpSession and ServletContext dependencies at core.Is there any reason why we don't implement HttpSession and ServletContext at core? cc @dkayiwa @ibacher

@HerbertYiga HerbertYiga force-pushed the RA-552 branch 2 times, most recently from 0165c4f to 4c3c4ba Compare March 4, 2021 01:41
@coveralls
Copy link

coveralls commented Mar 4, 2021

Coverage Status

Coverage increased (+53.3%) to 63.583% when pulling 0c6e572 on HerbertYiga:RA-552 into bae8500 on openmrs:master.

@HerbertYiga HerbertYiga force-pushed the RA-552 branch 2 times, most recently from 12e8ff9 to beacb42 Compare March 4, 2021 19:09
@sherrif10
Copy link
Member

its this @sherrif10 https://talk.openmrs.org/t/view-logged-in-users-is-empty/19348/4

Thanks @HerbertYiga for the link sorry for late response

@HerbertYiga HerbertYiga force-pushed the RA-552 branch 6 times, most recently from ddcdaea to fbe4dfc Compare March 9, 2021 13:20
@HerbertYiga
Copy link
Contributor Author

@dkayiwa how do things look on this pr,the travis breaks only for openjdk 10

public void getCurrentUsernames_shoulReturnAllCurrentUserNames() {
executeDataSet(USER_SET);
MockHttpSession session = new MockHttpSession();
User user = userService.getUser(5508);
Copy link
Member

Choose a reason for hiding this comment

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

Is this test simply about users stored in the database or logged in users?

Copy link
Contributor Author

@HerbertYiga HerbertYiga Mar 9, 2021

Choose a reason for hiding this comment

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

Is this test simply about users stored in the database or logged in users?

@dkayiwa ohh let me fix this, currently its about users stored in the database

@HerbertYiga
Copy link
Contributor Author

HerbertYiga commented Mar 10, 2021

hi @dkayiwa i have included logged in users for the tests


public class CurrentUsersTest extends BaseWebContextSensitiveTest {

protected static final String USER_SET = "org/openmrs/CurrentUserTest.xml";
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why this is not private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have fixed this


@Test

public void getCurrentUsernames_shoulReturnAllCurrentUserNames() {
Copy link
Member

Choose a reason for hiding this comment

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

Is the test method name reflecting what you are testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the test method name reflecting what you are testing?

i have fixed this too

@dkayiwa
Copy link
Member

dkayiwa commented Mar 10, 2021

@HerbertYiga did you run the web application and confirm that this is behaving as expected?

@HerbertYiga
Copy link
Contributor Author

did you run the web application and confirm that this is behaving as expected?

@dkayiwa my plan around this is to access the introduced method from reference application module so that i create a view there, could this be a right direction?

@HerbertYiga
Copy link
Contributor Author

hi @dkayiwa tested this with a running instance of openmrs and i see now the CurrentUsers.addUser() from core is used,i have added the current screenshot to the ticket id

@dkayiwa
Copy link
Member

dkayiwa commented May 30, 2021

@HerbertYiga which method in core calls CurrentUsers.addUser()?

@HerbertYiga
Copy link
Contributor Author

HerbertYiga commented Jun 2, 2021

hi @dkayiwa i have changed the implementation of this and now i implement the UserSessionListener,tested things on a running instance with legacy ui module and things run fine,the current screen shot can be found on the ticket id thanks

@dkayiwa
Copy link
Member

dkayiwa commented Jun 2, 2021

Did you see the travis build failure?

@HerbertYiga HerbertYiga force-pushed the RA-552 branch 7 times, most recently from efec5fb to a624c31 Compare June 2, 2021 13:44
@HerbertYiga
Copy link
Contributor Author

Did you see the travis build failure?

hi @dkayiwa ,i fixed the breaking part, re-tested on a running instance things run as required, ie (when a user logs in ,he is added on the list and when he logs out ,he is removed from the list of current users)
screen shot on the ticket id

@HerbertYiga
Copy link
Contributor Author

hi @dkayiwa is this good enough to be merged in?

@HerbertYiga
Copy link
Contributor Author

thanks for the review @ibacher

@ibacher
Copy link
Member

ibacher commented Jun 24, 2021

@HerbertYiga So, I think the code here looks decent enough for what it needs to do, but there are two things we'll also need to complete this work:

  1. When a user's session times out, it doesn't necessarily trigger the logout event. This is because the session is at the web layer and not at the application layer. In the legacy-ui, this was handled by [this https://github.com/openmrs/openmrs-module-legacyui/blob/c9bb7b8ba008573568f46cb2d09ca987d309683d/omod/src/main/java/org/openmrs/web/SessionListener.java#L40), but, as you can see, that implementation actually depends on the session identifier an not necessarily the user name. (Note, too, on this that it might be possible to have more than one user associated with a given session.
  2. Notice how in the legacy version of this feature there's a special case to handle the case where the username is blank. We also probably need to have something like that.

@HerbertYiga
Copy link
Contributor Author

@HerbertYiga So, I think the code here looks decent enough for what it needs to do, but there are two things we'll also need to complete this work:

  1. When a user's session times out, it doesn't necessarily trigger the logout event. This is because the session is at the web layer and not at the application layer. In the legacy-ui, this was handled by [this https://github.com/openmrs/openmrs-module-legacyui/blob/c9bb7b8ba008573568f46cb2d09ca987d309683d/omod/src/main/java/org/openmrs/web/SessionListener.java#L40), but, as you can see, that implementation actually depends on the session identifier an not necessarily the user name. (Note, too, on this that it might be possible to have more than one user associated with a given session.
  2. Notice how in the legacy version of this feature there's a special case to handle the case where the username is blank. We also probably need to have something like that.

Thanks @ibacher let me look through this

@github-actions
Copy link

tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed.

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare!

This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side.
Please note that this is an automated message and we might very well be the reason why there has not been any activity lately. We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs.

If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :)
If you would like to continue working on it or require help from us please remove the stale label and respond by commenting on the issue.

@github-actions github-actions bot added the Stale label Nov 30, 2021
@HerbertYiga
Copy link
Contributor Author

getting some time for this

@github-actions github-actions bot removed the Stale label Dec 1, 2021
@dkayiwa dkayiwa closed this Mar 22, 2023
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.

6 participants