onaio / rdt-standard

RDT Open Guidelines Application
Apache License 2.0
1 stars 5 forks source link

User is able to enter data in forms before session expiry flow could execute given connectivity is there #958

Closed mwkhan1983 closed 3 years ago

mwkhan1983 commented 3 years ago

Either user should be able to save data entered upon submit or delay should not be long enough to allow user to keep entering data.

vincent-karuri commented 3 years ago

Another option would be not to logout the user if they are filling out a form. Set the current activity in the onCreate of the CovidJsonFormActivity and clear it onStop.

Then check what activity is currently visible before deciding to logout or not. This may be an easier and more efficient lift.

owais-vd commented 3 years ago

I have checked by setting up the activity in CovidJsonFormActivity but it's not working because there is a process before starting the activity and it takes a time to load and in the meantime logout method called before setting up the activity.

vincent-karuri commented 3 years ago

@syedowaisali90 what process are you referring to?

owais-vd commented 3 years ago

https://github.com/onaio/rdt-standard/blob/master/rdt-open-reader/src/main/java/io/ona/rdt/util/FormLauncher.java#L30

vincent-karuri commented 3 years ago

what do is meant by it's not working? what about invoking this function makes it not work?

owais-vd commented 3 years ago

it's just processed all the components before starting up the form activity and it takes some time that' why we are showing the loader.

vincent-karuri commented 3 years ago

what exactly is the issue with that? does logout happen after the form is already launched?

owais-vd commented 3 years ago

yes, logout happens after the form is already lunched https://drive.google.com/file/d/15BnnMpFqrg6qdLn0R3Si2eeweMxQGqge/view

vincent-karuri commented 3 years ago

is this after making the change here https://github.com/onaio/rdt-standard/issues/958#issuecomment-821310139? Why would that happen?

owais-vd commented 3 years ago

https://github.com/onaio/rdt-standard/issues/958#issuecomment-821310139 this change won't work as i mentioned here https://github.com/onaio/rdt-standard/issues/958#issuecomment-822229869

vincent-karuri commented 3 years ago

the video you shared, https://github.com/onaio/rdt-standard/issues/958#issuecomment-826786121 is it after making the change or not?

owais-vd commented 3 years ago

No

vincent-karuri commented 3 years ago

@syedowaisali90 could you provide a detailed description or short video of what happens when you apply the change? I don't currently understand what is meant by it doesn't work.

owais-vd commented 3 years ago

Here is a scenario:

  1. The user has resumed profile activity and at the same time, userAuth has triggered in the background.
  2. Let assume userAuth in progress and meanwhile, a user tried to open a form.
  3. Now showing a form loader.
  4. Before starting the form activity i.e CovidJsonFormActivity. userAuth gets completed.
  5. At that moment in userAuth tried to get a current from RDTApplication class.
  6. But currently we don't have any activity in the RDTApplication class. Because of form launch still in progress.
  7. Now userAuth tried to logout and after that startActivity method get called and launched the form.
  8. When the form launched then we set the current activity in RDTApplication class.
  9. But setting up the activity is useless because userAuth has been already triggered.
vincent-karuri commented 3 years ago

Cool, I still don't see why the service having already started is an issue because if at the point it's calling logout the form activity is already launched/visible, then the form activity would have already registered itself as the current activity and logout should not happen. Unless you're saying logout happens while in 3 (the spinner is showing).

Are you able to reproduce forced logout when the form is showing for this scenario https://github.com/onaio/rdt-standard/issues/958#issuecomment-827400624?

owais-vd commented 3 years ago

I mean this Unless you're saying logout happens while in 3 (the spinner is showing). at this moment we don't have a CovidJsonFormActivity in RDTApplication class and this happened frequently. As you have seen here https://github.com/onaio/rdt-standard/issues/958#issuecomment-826786121

vincent-karuri commented 3 years ago

https://github.com/onaio/rdt-standard/issues/958#issuecomment-826786121 shows that logout happens while the form is already launched, not while the spinner is showing.

owais-vd commented 3 years ago

done https://github.com/onaio/rdt-standard/pull/959#discussion_r621395542

mwkhan1983 commented 3 years ago

@vincent-karuri @syedowaisali90 Please see attached video. Issue is occurring again. User is filling in the data not knowing session has expired and when complete form is filled and submitted, data is not saved as user has already logged out in the background.

The authentication check once the form opens was working well. It was not allowing user to enter data and logging out. Not sure why we changed that.

vincent-karuri commented 3 years ago

@mwkhan1983 What was merged was supposed to ensure that the app doesn't log out when a form is open . If this is still happening, we need to list the steps to reproduce that behaviour and get it definitively fixed . I'm not sure what was working before that is not working now since that is the bug described in the issue (logging out when filling out a form).

mwkhan1983 commented 3 years ago

@vincent-karuri App doesn't logout when form is open but when you submit the form, data is not saved because user is already logged out in the background. This is an issue because user would not know that despite submitting data, it is not saved.

So original issue described asked for one of the two things to happen:

  1. App doesn't logout when form is open and saves data on form submission.
  2. App does logout when form is opened but that should not take long enough for user to be able to enter substantial data.

Through authorization check when opening form, we were able to achieve 2.

vincent-karuri commented 3 years ago
  1. I'm not sure what is meant by logged out in the background, could you elaborate?
  2. How was it verified that the data is not saved?
  3. Could you provide steps to reproduce this
  4. How does adding authz when launching a form prevent 2 from occurring? authz is an asynchronous process meaning it allows the form to open while still running in the background (so no guarantees it completes before the form launches). It was removed because it doesn't make too much sense to call this on every form launch (resource intensive).
mwkhan1983 commented 3 years ago
  1. Did you see the video shared in this comment https://github.com/onaio/rdt-standard/issues/958#issuecomment-830084467? When you see it, it will be clear that the new thread starts when form is launched while auth activity runs on another thread. When form thread completes on submitting the form, user is directly taken to a logout screen.
  2. Simple. Login again and see the data submitted. No data found in history
  3. Steps: login. Wait for 20-25 minutes (session timeout). Resume app. Open any of the forms. Complete and submit form. User is taken to login screen on form submission. Login again. Data submitted is not saved and not shown in history. All these steps are pretty clear to @syedowaisali90 . He is able to reproduce this issue
  4. After Owais applied auth on forms, I have tested it thoroughly. it was working fine, logging out user with a message that session is expired after from is opened. Auth was checking after form is opened in the same thread. and applying on every form is what makes sense. Because patient view, profile view are read-only. It doesn't make a difference if user is interacting with those screens even if session is timedout. However, when user is interacting with forms entering data, that is where the issue is. User not knowing session is timed-out and keep entering data. At the end when form is submitted, user realize that all data that is entered is lost/not saved because session wasn't there.
vincent-karuri commented 3 years ago

@mwkhan1983 one cannot make the conclusion in 1 by looking at the video. There should be no "background logout" if the current activity is a form activity (has @syedowaisali90 confirmed the "background logout" during debugging?).

I think all steps should ideally be logged on the issue so that they are visible to everyone involved.

Authz has always been done in a background thread. Again, this is not a conclusion that can be made by looking at the video. Even if we revert the authz to happen on every form launch, it's simply coincidental that the authz check happens fast enough before too many details are entered into the form and is not a definitive fix imo.

@syedowaisali90 could you check why logout happens soon after submission, and also explore if this is a problem with the form activity being cleared before submission is complete?

owais-vd commented 3 years ago

@vincent-karuri could you test the app from step 3 here https://github.com/onaio/rdt-standard/issues/958#issuecomment-831130836 so that you see the real problem.

vincent-karuri commented 3 years ago

@syedowaisali90, @mwkhan1983 has described what is happening behaviorally. We should try figure out why what is happening in code. There may be other reasons but two things to keep in mind is:

  1. Figure out whether logout is happening because the current activity is cleared and VerifyAuthorizationTask runs when saving a form
  2. Or sync happens when the form is open, triggering forced-logout - we don't check the current activity before triggering sync atm
owais-vd commented 3 years ago

@vincent-karuri Here is the scenario https://github.com/onaio/rdt-standard/issues/958#issuecomment-827400624. The main reason, why it's happening just because syncUtils.logoutUser(); get called called during the form loading.

When we try to open a form so it takes some time to open a CovidJsonFormActivity so that's why I have used caller activity as a check instead of CovidJsonFormActivity.

Currently, we have only two caller activity the one is CovidPatientProfileActivity and the second is CovidPatientRegisterActivity and I have tested that now it's working fine.