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

Ekirjasto 110 add token refresh #180

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

natlibfi-burger
Copy link
Contributor

What's this do?
Makes the automatic refresh implementation to books and magazines, so
that if 401 HTTP error is noticed, the app tries to refresh the token
immediately.

If this is successful, the user sees a slightly longer loading screen or a screen refresh.
If unsuccessful, the user is informed about expired session and is then
shown the login page.

The refresh itself is a copy of the ekirjasto login function that is used on login after
gaining the ekirjasto token from loikka. The only difference is that instead of the
ekirjasto token, we use the (most likely old) circulationToken to trigger the refresh.
The token is then updated to the db and credentials, from where the other parts use it from.
As this is considered a new login, it triggers a refresh for all the views.

The book related refreshes are triggered and handled by the controller, but
the refresh is possible to be triggered from other places as well.
The refresh trigger was put into controller, as it kept the fragments as
oblivious to the background happenings as possible, and it was more
simple to put into the controller, that is shared, rather than multiple
different viewModels.

The magazines fragment and the dependents view trigger the
refresh on their own. This is because the controller has all the needed
information but it only handles book related actions, which magazines
and dependents are not.

When the refresh is started, the account is put to WaitingForExternalAuth state,
and if the login is successful, it turns into logged in with the new credentials.
If logout happens due to expired token, and the token can not be
refreshed, the user is logged out without actually deleting the user's
data, as deleting the data would only cause irritation if the logout is
not spesifically triggered by the user.

The popup is coded nearly identically into three different places, which is not the best way to handle it,
causing there also to be repetive strings to be translated. Could be changed later into a better solution.

The way the dependents fragment was coded proved difficult to make
asynchronous so that the refresh triggering would work correctly, so
the viewmodel is changed to match the magazine fragment more, and thus
there is new files that handle the http requests and the fragment now
has a state. The state could also be used to handle the error messaging
and other things seen in the fragment, but kept the old implementation
of that as it would've taken more time to ensure the visiblilities of
all the different parts were correct. The changes also caused a new service to be added.

The 401 can be returned, but not handled in PatronUserProfiles. This is because the call is not made on its own, and always after that call, the following call will also throw 401 that we do catch.

TO NOTE: The loans are also always loaded on app start, so if the app is closed, and not on the background running, just opening the app is enough to refresh the token.

Why are we doing this? (w/ JIRA link if applicable)

This update makes it possible for the user to remain logged into their account indefinedly, as long as they do active things in the application, such as loan, reserve, return or loan, books, or look at magazines. This makes the use of application easier, as the user doesn't have to constantly log in again, making the user experience smoother.

https://jira.it.helsinki.fi/browse/EKIRJASTO-110

How should this be tested? / Do these changes have associated tests?
This can be tested by logging into the test environment, and trying the following actions, after waiting a few minutes:
Loan a book
Return a book
Load a book
Open magazines
Reload your own loans (pulling down in the loans view to refresh the view)
Get dependents

The availability time of the token is currently 120 seconds, after which the token is expired and the server returns 401.

The failure to refresh is a little harder to test.
To simulate the situation, in the controller, the lines 474-483 in the Failure condition can be copied to the Success condition above it. This makes the behavior not fully correct, as we log in and then instantly log out, which triggers multiple uneccessary refreshes, but the popup should pop up (at least once) in the catalog, single book, and magazines views, after refreshing after the 120 seconds.

Dependencies for merging? Releasing to production?

Accidentally also updated two libraries, but they don't seem to affect anything.

Does this require updates to old Transifex strings? Have the translators been informed?

There are new transifex strings for the popup, as well as a fix to one older one. Translators are aware of these changes.

Added functions to handle refresh. Marked different
spots from where to possibly call the function from.
Created a button for accessToken refresh into the settings page.Removed
some unused code.
Make the automatic refresh implementation to books and magazines, so
that if 401 HTTP error is noticed, the app tries to refresh the token
immediately.
If this is successful, the user sees a slightly longer
loading screen or a screen  refresh.
If unsuccessful, the user is informed about expired session and is then
shown the login page.
Most of the refreshes  are triggered and handled by the controller, but
the refresh is possible to be triggered from other places as well.
Fir instance, the magazines fragment and the dependents view trigger the
refresh on their own. This is because the controller has all the needed
information but it only handles book related actions, which magazines
and dependents are not.
But the code was put into controller, as it kept the fragments as
oblivious to the background happenings as possible, and it was more
simple to put into the controller, that is shared, rather than multiple
different viewModels.
If logout happens due to expired token, and the token can not be
refreshed, the user is logged out without actually deleting the user's
data, as deleting the data would only cause irritation if the logout is
not spesifically triggered by the user.

The way the dependents fragment was coded proved difficult to make
asynchronous so that the refresh triggering would work correctly, so
the viewmodel is changed to match the magazine fragment more, and thus
there is new files that handle the http requests and the fragment now
has a state. The state could also be used to handle the error messaging
and other things seen in the fragment, but kept the old inplementation
of that as it would've taken more time to ensure the visiblilities of
all the different parts were correct.

Also deleted the refresh button from the settings page.

REF: EKIRJASTO-110
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.

1 participant