TRUNK-6235: auto deactivate users after XX days of inactivity

[dicksonmulli]

Description of what I changed

• Creating a task for auto retiring users after XX days of inactivity.
• Setting number-of-days-to-retire-users property in global properties.
• Adding last login time in user properties

Issue I worked on

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

Checklist: I completed these to help reviewers :)

• 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

[dkayiwa]

Can we also look into the build failures?

[dkayiwa]

@dicksonmulli can you look into the build failures.

[dkayiwa]

Not really, but we need to allow this ability. For instance, when an admin wants to test this feature, it will come in handy to set say half a day or few hours.

We generally tend to avoid being too permissive. Especially when it has not been explicitly requested for. 😊

[dicksonmulli]

We generally tend to avoid being too permissive. Especially when it has not been explicitly requested for.

I see, however, I genuinely think this ability will be beneficial not only to testing but also preventing errors when the user accidentally adds numbers with decimal places.

[dkayiwa]

I see, however, I genuinely think this ability will be beneficial not only to testing but also preventing errors when the user accidentally adds numbers with decimal places.

For preventing errors, our error handling should take care of it.
Avoiding a builtin library and add our custom code that increases the maintenance burden, is not worth it, when all this is done in the name of supporting decimal places, which is not part of the user requirements. 😊

[dicksonmulli]

Did you intentionally set this to package-private access?

Yes, I did this to enable us to call these methods from the unit tests.

[Note] It could be that you can't see my replies anymore. I am not sure what changed.

[dkayiwa]

Yes, I did this to enable us to call these methods from the unit tests.

Ummmmm, it does not generally smell good, to make methods more visible than they should, just for the sake of testing them. We naturally test them via their public contract. Or in the worst case, at least access them via reflection in the tests. 😊

[dkayiwa]

Shouldn't the above be private?

[dicksonmulli]

I was using it in tests but I can still use anyString() instead of passing the actual reason.

[dkayiwa]

Will this test fail if i remove the above line of adding the super user role?

[dkayiwa]

@dicksonmulli did you see the above comment?

[dicksonmulli]

Commenting should be working now.

Regarding the above concern, kindly refer to the other reply that I shared with you on slack.

[dkayiwa]

It is better to share the reply here. These are public comments that are usually read by other community members who may join the discussion.

[dicksonmulli]

#4650 (comment)

[dicksonmulli]

Ideally it should fail but the reason it won't fail when you remove this line is because the algorithm will check if the inactivity exceeds the number of days to retire, of which it doesn't, unless we add unnecessary stubbings of a last login time or created date - e.g. when(userService.getLastLoginTime(any())).thenReturn(String.valueOf(System.currentTimeMillis() - TWO_DAYS_IN_MILLISECONDS));
Note that these unnecessary stubbing would cause an error.

[dkayiwa]

I have not properly understood your response. Let me ask differently, if i removed the line which adds the super user role to the user, will this test fail?

[dkayiwa]

Will this test fail if i remove the above line of setRetired(true)?

[dkayiwa]

@dicksonmulli did you see the above comment?

[dkayiwa]

Do you have a response to the above comment?

[dicksonmulli]

I shared the response here:
#4650 (comment)

[dkayiwa]

If i do not retire the user in this test, and the test still passes, what then would i be testing for? 😊

[dicksonmulli]

Shouldn't the above be private?

I was using it in tests but I can still use anyString() instead of passing the actual reason.

[dicksonmulli]

Will this test fail if i remove the above line of setRetired(true)?

Ideally it should fail but the reason it won't fail when you remove this line is because the algorithm will check if the inactivity exceeds the number of days to retire, of which it doesn't, unless we add unnecessary stubbings of a last login time or created date - e.g. when(userService.getLastLoginTime(any())).thenReturn(String.valueOf(System.currentTimeMillis() - TWO_DAYS_IN_MILLISECONDS));
Note that these unnecessary stubbing would cause an error.

Will this test fail if i remove the above line of adding the super user role?

Same case as the above

[dkayiwa]

I was using it in tests but I can still use anyString() instead of passing the actual reason.

Would this catch a bug which changes the expected string to a different one?

[dicksonmulli]

Would this catch a bug which changes the expected string to a different one?

I have updated the tests with the actual string. This may have resulted to duplication of code but on the positive side, we achieve separation of concerns.

[dkayiwa]

@dicksonmulli can you look at these merge conflicts?