RA-552:Adding the View Logged in Users functionality to core #3706

HerbertYiga · 2021-03-01T19:36:22Z

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

Adding the View Logged in Users functionality to core

HerbertYiga · 2021-03-01T19:44:01Z

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

+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
+import javax.servlet.http.HttpSession;


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?

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

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

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

@dkayiwa awesome, let me go on and do that

Remember to test and confirm that it all works.

sherrif10 · 2021-03-01T20:08:14Z

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

HerbertYiga · 2021-03-01T20:48:31Z

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

sherrif10 · 2021-03-01T20:54:49Z

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

HerbertYiga · 2021-03-01T20:56:35Z

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

dkayiwa · 2021-03-02T19:02:44Z

@HerbertYiga did you see the travis build failure?

HerbertYiga · 2021-03-03T05:42:42Z

@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

coveralls · 2021-03-04T02:12:55Z

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

sherrif10 · 2021-03-04T19:47:50Z

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 · 2021-03-09T14:19:30Z

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

dkayiwa · 2021-03-09T14:40:21Z

web/src/test/java/org/openmrs/web/filter/update/util/CurrentUsersTest.java

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


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

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 · 2021-03-10T11:23:21Z

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

dkayiwa · 2021-03-10T13:56:12Z

web/src/test/java/org/openmrs/web/filter/update/util/CurrentUsersTest.java

+
+public class CurrentUsersTest extends BaseWebContextSensitiveTest {
+
+	protected static final String USER_SET = "org/openmrs/CurrentUserTest.xml";


Any reason why this is not private?

i have fixed this

dkayiwa · 2021-03-10T13:57:14Z

web/src/test/java/org/openmrs/web/filter/update/util/CurrentUsersTest.java

+
+	@Test
+
+	public void getCurrentUsernames_shoulReturnAllCurrentUserNames() {


Is the test method name reflecting what you are testing?

Is the test method name reflecting what you are testing?

i have fixed this too

dkayiwa · 2021-03-10T14:00:06Z

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

HerbertYiga · 2021-03-10T20:03:13Z

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 · 2021-05-30T21:55:52Z

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 · 2021-05-30T22:00:59Z

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

HerbertYiga · 2021-06-02T01:44:18Z

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 · 2021-06-02T02:29:25Z

Did you see the travis build failure?

HerbertYiga · 2021-06-02T18:30:37Z

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 · 2021-06-16T17:20:46Z

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

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

HerbertYiga · 2021-06-24T18:32:04Z

thanks for the review @ibacher

ibacher · 2021-06-24T19:00:25Z

@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:

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.
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 · 2021-06-24T19:03:36Z

@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:

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.

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 · 2021-11-30T00:16:21Z

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.

HerbertYiga · 2021-11-30T10:32:12Z

getting some time for this

HerbertYiga force-pushed the RA-552 branch from 3c08b65 to c58ff1c Compare March 1, 2021 19:40

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

HerbertYiga commented Mar 1, 2021

View reviewed changes

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

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

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

dkayiwa reviewed Mar 9, 2021

View reviewed changes

HerbertYiga force-pushed the RA-552 branch from 0bcf600 to a6b2137 Compare March 10, 2021 09:46

dkayiwa reviewed Mar 10, 2021

View reviewed changes

HerbertYiga force-pushed the RA-552 branch from da3bd49 to 29bf57f Compare March 10, 2021 19:52

HerbertYiga force-pushed the RA-552 branch from fcdf1df to 3a46d26 Compare June 2, 2021 01:34

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

RA-552:Adding the View Logged in Users functionality to core

0c6e572

HerbertYiga force-pushed the RA-552 branch from a624c31 to 0c6e572 Compare June 2, 2021 17:57

ibacher reviewed Jun 24, 2021

View reviewed changes

api/src/main/java/org/openmrs/util/CurrentUsers.java Outdated Show resolved Hide resolved

Update api/src/main/java/org/openmrs/util/CurrentUsers.java

fa74a8a

ibacher reviewed Jun 24, 2021

View reviewed changes

api/src/main/java/org/openmrs/util/CurrentUsers.java Show resolved Hide resolved

Update api/src/main/java/org/openmrs/util/CurrentUsers.java

e637bf6

ibacher reviewed Jun 24, 2021

View reviewed changes

api/src/main/java/org/openmrs/util/CurrentUsers.java Outdated Show resolved Hide resolved

Update api/src/main/java/org/openmrs/util/CurrentUsers.java

8114fe6

github-actions bot added the Stale label Nov 30, 2021

github-actions bot removed the Stale label Dec 1, 2021

dkayiwa closed this Mar 22, 2023


		public class CurrentUsersTest extends BaseWebContextSensitiveTest {

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


		@Test

		public void getCurrentUsernames_shoulReturnAllCurrentUserNames() {

RA-552:Adding the View Logged in Users functionality to core #3706

RA-552:Adding the View Logged in Users functionality to core #3706

Conversation

HerbertYiga commented Mar 1, 2021

HerbertYiga Mar 1, 2021 • 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

sherrif10 commented Mar 1, 2021

HerbertYiga commented Mar 1, 2021

sherrif10 commented Mar 1, 2021

HerbertYiga commented Mar 1, 2021

dkayiwa commented Mar 2, 2021

HerbertYiga commented Mar 3, 2021

coveralls commented Mar 4, 2021 • edited Loading

sherrif10 commented Mar 4, 2021

HerbertYiga commented Mar 9, 2021

Choose a reason for hiding this comment

HerbertYiga Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

HerbertYiga commented Mar 10, 2021 • 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

dkayiwa commented Mar 10, 2021 • edited Loading

HerbertYiga commented Mar 10, 2021

HerbertYiga commented May 30, 2021

dkayiwa commented May 30, 2021 • edited Loading

HerbertYiga commented Jun 2, 2021 • edited Loading

dkayiwa commented Jun 2, 2021

HerbertYiga commented Jun 2, 2021

HerbertYiga commented Jun 16, 2021

HerbertYiga commented Jun 24, 2021

ibacher commented Jun 24, 2021

HerbertYiga commented Jun 24, 2021

github-actions bot commented Nov 30, 2021

HerbertYiga commented Nov 30, 2021

HerbertYiga Mar 1, 2021 •

edited

Loading

coveralls commented Mar 4, 2021 •

edited

Loading

HerbertYiga Mar 9, 2021 •

edited

Loading

HerbertYiga commented Mar 10, 2021 •

edited

Loading

dkayiwa commented Mar 10, 2021 •

edited

Loading

dkayiwa commented May 30, 2021 •

edited

Loading

HerbertYiga commented Jun 2, 2021 •

edited

Loading