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

feat(neon_framework): Enable viewing and editing profile properties #2345

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Closes #2326
Closes #2327

Currently the password confirmation is triggered on the UI side and not the bloc side which I do not like, but the alternative would be to register a global callback that would be called in the blocs. I'm not a fan of that either, so I wonder if you have a better way to implement this.
The main point of the PR is to implement the password confirmation, editing the profile is only secondary and used as an example. If the later needs more work I'd be pleased to already merge the password confirmation in case the discussions around that are already resolved.

@provokateurin provokateurin requested a review from Leptopoda August 3, 2024 16:12
Copy link

codecov bot commented Aug 3, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 58 lines in your changes missing coverage. Please review.

Project coverage is 26.41%. Comparing base (60d46a3) to head (4c877ac).
Report is 4 commits behind head on main.

Files Patch % Lines
...neon_framework/lib/src/pages/account_settings.dart 0.00% 58 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2345      +/-   ##
==========================================
+ Coverage   26.32%   26.41%   +0.09%     
==========================================
  Files         362      365       +3     
  Lines      136541   136701     +160     
==========================================
+ Hits        35940    36112     +172     
+ Misses     100601   100589      -12     
Flag Coverage Δ *Carryforward flag
cookie_store 100.00% <ø> (ø) Carriedforward from 60d46a3
dashboard_app 96.05% <ø> (ø)
dynamite 31.08% <ø> (ø) Carriedforward from 60d46a3
dynamite_end_to_end_test 61.84% <ø> (ø) Carriedforward from 60d46a3
dynamite_runtime 85.40% <ø> (ø) Carriedforward from 60d46a3
neon_dashboard 96.05% <ø> (ø) Carriedforward from 60d46a3
neon_framework 47.97% <71.42%> (+2.66%) ⬆️
neon_http_client 93.28% <ø> (ø) Carriedforward from 60d46a3
neon_notifications 100.00% <ø> (ø) Carriedforward from 60d46a3
neon_talk 99.45% <ø> (ø) Carriedforward from 60d46a3
nextcloud 22.34% <ø> (ø) Carriedforward from 60d46a3
notifications_app 100.00% <ø> (ø)
sort_box 90.90% <ø> (ø) Carriedforward from 60d46a3
talk_app 99.09% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...ges/neon_framework/lib/src/blocs/user_details.dart 100.00% <100.00%> (ø)
...framework/lib/src/utils/password_confirmation.dart 100.00% <100.00%> (ø)
...mework/lib/src/widgets/settings_profile_field.dart 100.00% <100.00%> (ø)
...work/lib/src/widgets/settings_profile_section.dart 100.00% <100.00%> (ø)
...neon_framework/lib/src/pages/account_settings.dart 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

@provokateurin provokateurin force-pushed the feat/neon_framework/profile-settings branch from ad33020 to cb4e86d Compare August 4, 2024 10:44
@provokateurin provokateurin force-pushed the feat/neon_framework/profile-settings branch from cb4e86d to 2789fd8 Compare August 16, 2024 14:51
@provokateurin provokateurin force-pushed the feat/neon_framework/profile-settings branch from 2789fd8 to 4c877ac Compare August 17, 2024 08:58
@provokateurin
Copy link
Member Author

With the account repository merged and used everywhere, can we now take a look at this PR @Leptopoda?
I remember you said this would be perfect for the account repository. While I agree that most logic can be moved into it, I'm not sure how you want to tackle the problem of password confirmation having to interact with the UI.
The only way I'd see would be an async callback in the repository that is passed to it which allows triggering the password confirmation dialog.
I don't see another reasonable way to do this, other than using a callback or keeping the password confirmation logic entirely in the UI.
That said, other repositories will also need to be able to do password confirmations, so they'd either need to get the same callback or call the account repository to do it which might be a bit overkill if that is the only use of the account repository.

@Leptopoda
Copy link
Member

I don't think repositories should have any callbacks.
They can emit a stream of data the UI can subscribe to, but should never itself be able to call into UI code.

My Idea for this would be:

  • have a bool validatePassword(String password) method in the AccountRepository. This would handle the API calls, error handling ...
  • have a package:bloc bloc that handles the state of the UI element doing the password confirmation (text input, state handling ...). This will call into the account repo to do the actual validation
  • have a UI component that handles the password confirmation flow (probably the dialog you've already built). The UI state (error message, submit buttons, loading UI) would be handled here.

Instead of the page view structure we have for pages, I'd create a class MyCoolDialog
that handles the bloc insertion using a provider (similar to the page), and a MyCoolDialogView that handles the ui itself (similar to the view). Depending on their size they can both live in a single file (unlike the pages and views that I prefer separated). The same file can then also contain a showMyCoolDialog method that calls showDialog underneath.

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.

Enable editing of personal profile information Support features requiring password confirmation
2 participants