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

fremartini · 2023-10-03T06:24:40Z

closes #169

ghost · 2023-10-03T06:25:25Z

👇 Click on the image for a new way to code review

Legend

codecov · 2023-10-03T06:32:09Z

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 ⚠️
...tion/presentation/cubits/authentication_cubit.dart	94.11%	2 Missing ⚠️
...res/session/data/models/session_details_model.dart	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            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	Coverage Δ
unittests	`73.77% <93.91%> (+1.41%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marfavi

The feature can be implemented with 5-10 changed files instead of 30. Remove any code refactoring from your feature branch before proceeding.

lib/core/external/date_service.dart

lib/features/authentication/data/datasources/authentication_local_data_source.dart

lib/features/authentication/data/intercepters/authentication_interceptor.dart

lib/features/authentication/domain/entities/authenticated_user.dart

fremartini · 2023-11-19T14:29:10Z

#552

lib/features/session/data/models/session_details_model.dart

marfavi · 2023-12-09T14:58:41Z

I have a few thoughts:

`AuthenticationCubit` vs server-side

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

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).
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.
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 AnalogIO/analog-core#238 before attempting to implement this client-side.

marfavi

Address my previous comment.

marfavi · 2023-12-09T13:25:11Z

lib/core/widgets/components/dropdown.dart

-  final FilterCategory value;
-  final List<DropdownMenuItem<FilterCategory>>? items;
-  final void Function(FilterCategory?) onChanged;
+  final T value;


Give a more descriptive name for the type T

Suggested change

final T value;

final DropdownItem value;

marfavi · 2023-12-09T13:27:54Z

lib/core/widgets/components/dropdown.dart

+  final TextStyle? textStyle;
+  final Color? dropdownColor;


Make required with no default value. What I suspect you really want is a way to change the colour of the text, in which case you can also change the TextStyle field into a Color field instead.

Suggested change

final TextStyle? textStyle;

final Color? dropdownColor;

final Color textColor;

final Color dropdownColor;

marfavi · 2023-12-09T13:51:24Z

lib/features/settings/presentation/widgets/settings_list_entry.dart

@@ -9,22 +8,26 @@ class SettingListEntry extends StatelessWidget {
  final bool destructive;
  final void Function()? onTap;
  final ListEntrySide sideToExpand;
+  final bool overrideDisableBehaviour;


Don't override intended disabling behavior.

marfavi · 2023-12-09T13:53:45Z

lib/service_locator.dart

+void initSession() {
+  // bloc
+  const List<SessionTimeout> entries = [
+    ('2 hours', Duration(hours: 2)),


Don't put actual configuration data in the service locator

marfavi · 2023-12-09T16:33:22Z

lib/core/widgets/components/dropdown.dart

@@ -2,17 +2,21 @@ import 'package:coffeecard/core/styles/app_colors.dart';
 import 'package:coffeecard/core/styles/app_text_styles.dart';
 import 'package:flutter/material.dart';

-class Dropdown<FilterCategory> extends StatelessWidget {
+class Dropdown<T> extends StatelessWidget {


Don't change the intended use case for this widget.

fremartini added 8 commits October 2, 2023 15:49

secure_storage --> authentication_local_data_source

df7b3f8

more refactoring

b3c70bc

implementation without configuration

5842c52

add session timeout ui to settings

2dda1f1

configuration implementation

7c4abbb

tests

65cb4e1

fixture

4698cf5

more test cases

a6ea67d

fremartini requested a review from a team October 3, 2023 06:24

shorthand function call

b08a641

fremartini enabled auto-merge (squash) October 12, 2023 14:18

fremartini changed the base branch from develop to main October 15, 2023 09:39

marfavi requested changes Oct 19, 2023

View reviewed changes

simplify

bcb49e9

fremartini requested a review from marfavi October 29, 2023 11:16

fremartini and others added 2 commits November 9, 2023 21:17

Merge remote-tracking branch 'origin/main' into frem/sign_out_timer

94686ca

Merge branch 'main' into frem/sign_out_timer

ca6ac43

fremartini marked this pull request as draft November 11, 2023 08:07

auto-merge was automatically disabled November 11, 2023 08:07
Pull request was converted to draft

simple routing

8d11d7e

marfavi changed the title ~~Sign user out after 2 hours/Never~~ feat: sign user out after 2 hours/Never Nov 16, 2023

fremartini added 6 commits November 22, 2023 17:20

merge origin/main

4505f97

FUCKING MINIMAL

058d48c

and a comma

ec8f80d

Something works i guess

92dec1c

I know what I am doing. Trust me.

4c6dec2

Tests also need to be written

5844a43

remove fixtures

be36bbf

fremartini marked this pull request as ready for review November 27, 2023 15:49

fremartini requested a review from a team November 27, 2023 15:50

fremartini enabled auto-merge (squash) November 27, 2023 15:50

fredpetersen reviewed Nov 30, 2023

View reviewed changes

lib/features/session/data/models/session_details_model.dart Outdated Show resolved Hide resolved

lib/features/session/data/models/session_details_model.dart Outdated Show resolved Hide resolved

update function names

f698de7

fremartini requested a review from fredpetersen November 30, 2023 18:40

fremartini disabled auto-merge November 30, 2023 18:40

Merge branch 'main' into frem/sign_out_timer

fce0515

fremartini enabled auto-merge (squash) December 7, 2023 17:57

marfavi closed this Dec 9, 2023

auto-merge was automatically disabled December 9, 2023 14:58
Pull request was closed

fremartini reopened this Dec 9, 2023

marfavi requested changes Dec 9, 2023

View reviewed changes

marfavi closed this Mar 6, 2024

marfavi deleted the frem/sign_out_timer branch March 6, 2024 07:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fremartini commented Oct 3, 2023

ghost commented Oct 3, 2023 •

edited by ghost

Loading

Legend

codecov bot commented Oct 3, 2023 •

edited

Loading

marfavi left a comment

fremartini commented Nov 19, 2023

marfavi commented Dec 9, 2023

marfavi left a comment

marfavi Dec 9, 2023

marfavi Dec 9, 2023

marfavi Dec 9, 2023

marfavi Dec 9, 2023

marfavi Dec 9, 2023

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

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

Conversation

fremartini commented Oct 3, 2023

ghost commented Oct 3, 2023 • edited by ghost Loading

Legend

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

marfavi left a comment

Choose a reason for hiding this comment

fremartini commented Nov 19, 2023

marfavi commented Dec 9, 2023

AuthenticationCubit vs server-side

Other code considerations and UX

Next steps

marfavi left a comment

Choose a reason for hiding this comment

marfavi Dec 9, 2023

Choose a reason for hiding this comment

marfavi Dec 9, 2023

Choose a reason for hiding this comment

marfavi Dec 9, 2023

Choose a reason for hiding this comment

marfavi Dec 9, 2023

Choose a reason for hiding this comment

marfavi Dec 9, 2023

Choose a reason for hiding this comment

ghost commented Oct 3, 2023 •

edited by ghost

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

`AuthenticationCubit` vs server-side