AnalogIO / coffeecard_app

Cross-platform coffee card app for Cafe Analog
https://www.cafeanalog.dk/app
MIT License
6 stars 1 forks source link

feat: sign user out after 2 hours/Never #514

Closed fremartini closed 3 months ago

fremartini commented 9 months ago

closes #169

ghost commented 9 months ago
👇 Click on the image for a new way to code review #### [![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/AnalogIO/coffeecard_app/514/411ff573/fce05156861fe36933f8be548542da9cdb07c16e.svg)](https://app.codesee.io/r/reviews?pr=514&src=https%3A%2F%2Fgithub.com%2FAnalogIO%2Fcoffeecard_app) #### Legend CodeSee Map legend
codecov[bot] commented 9 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (bc0fa26) 72.36% compared to head (fce0515) 73.77%. Report is 2 commits behind head on main.

Files Patch % Lines
...sion/presentation/cubit/session_timeout_state.dart 63.63% 4 Missing :warning:
...tion/presentation/cubits/authentication_cubit.dart 94.11% 2 Missing :warning:
...res/session/data/models/session_details_model.dart 95.83% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #514 +/- ## ========================================== + Coverage 72.36% 73.77% +1.41% ========================================== Files 133 140 +7 Lines 1603 1716 +113 ========================================== + Hits 1160 1266 +106 - Misses 443 450 +7 ``` | [Flag](https://app.codecov.io/gh/AnalogIO/coffeecard_app/pull/514/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AnalogIO) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/AnalogIO/coffeecard_app/pull/514/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AnalogIO) | `73.77% <93.91%> (+1.41%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AnalogIO#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fremartini commented 7 months ago

https://github.com/AnalogIO/coffeecard_app/pull/552

marfavi commented 6 months ago

I have a few thoughts:

AuthenticationCubit vs server-side

I see a few issues with tracking session timeouts locally within AuthenticationCubit:

  1. Right now, the session timeout depends on the device's time (dateService.currentDateTime). This could get tricky if someone's device time is off. I'm thinking a server-side solution might be more foolproof, as it keeps the time check consistent and out of the user's hands (and our code).

  2. The code checks for session expiration only when the app starts up (appStarted method). What if the app stays open past the session timeout? It might be better if the app could actively monitor the session status and log out automatically when time’s up, even if it's still running.

  3. Adding session timeout management into AuthenticationCubit increases code complexity by quite a bit. Handling this on the server-side might simplify our code and let AuthenticationCubit focus just whether the user is logged in or not.

I think tackling session timeouts on the server could really streamline things. It might also make features like "auto-logout after X inactive hours" easier to manage since the server would just reset the session timer with each valid request.

Other code considerations and UX

Also, about the changes to Dropdown and SettingListItem - they seem to be doing things a bit differently now. We should probably stick to keeping these components simple to avoid any unexpected behaviors. For instance, we don't want any complex behaviour in a setting list item - see how we open a new page for other settings that requires text input or a selection from a list. How the app handles session timeouts UX-wise also needs to be discussed before we try for a new implementation. For instance, does the user even need to be able to choose the expiration time themselves?

Next steps

I appreciate the effort you've put into this PR, but for now, I'm closing this PR. To get this feature up and running we should discuss the implementation details further in https://github.com/AnalogIO/analog-core/issues/238 before attempting to implement this client-side.