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.

Sign up for GitHub

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

Jump to bottom

Add account loader #7166

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Add account loader #7166

merged 5 commits into from
Sep 25, 2023

Conversation

wmontwe
Copy link
Member

@wmontwe wmontwe commented Sep 12, 2023 •
edited
Loading

This adds an AccountLoader to retrieve current account information from the legacy code base. This will be used in the account edit feature.

@cketti
Copy link
Member

cketti commented Sep 12, 2023

I'm assuming this is in preparation for using the new UI to edit server settings. I feel like this is making things more complicated than they need to be.

To edit and check the server settings we don't need to know about the whole account (e.g. account options). So AccountIncomingConfigViewModel, AccountOutgoingConfigViewModel, and ServerValidationViewModel probably shouldn't use AccountStateRepository, but maybe a ServerSettingsStateRepository interface (that InMemoryAccountStateRepository can implement). For editing server settings we could then provide an AccountServerSettingsStateRepository that is accessing an existing account and is completely separate from InMemoryAccountStateRepository.

@wmontwe
Copy link
Member Author

wmontwe commented Sep 13, 2023 •
edited
Loading

I'm assuming this is in preparation for using the new UI to edit server settings. I feel like this is making things more complicated than they need to be.

To edit and check the server settings we don't need to know about the whole account (e.g. account options). So AccountIncomingConfigViewModel, AccountOutgoingConfigViewModel, and ServerValidationViewModel probably shouldn't use AccountStateRepository, but maybe a ServerSettingsStateRepository interface (that InMemoryAccountStateRepository can implement). For editing server settings we could then provide an AccountServerSettingsStateRepository that is accessing an existing account and is completely separate from InMemoryAccountStateRepository.

The complication is caused by OAuth and handling the authorization state, as it's seperate from the ServerSettings. Also the AccountIncomingConfigViewModel, AccountOutgoingConfigViewModel are using the email to prepopulate the username in case there are no settings yet (hard requirement?). I think my current approach is not the best to reuse the InMemoryAccountStateRepository for the edit feature. I give it a shot to refactor a little bit more.

@wmontwe wmontwe force-pushed the add_account_loader branch 2 times, most recently from 152bff8 to 5eb565f Compare September 15, 2023 15:56
@wmontwe
Copy link
Member Author

wmontwe commented Sep 15, 2023

I simplified the AccountLoader and it is now pretty much standalone not interacting with the InMemoryAccountStateRepository anymore.

@wmontwe
Copy link
Member Author

wmontwe commented Sep 18, 2023

Build related issues have been moved to #7178. Once merged this could be rebased.

@wmontwe wmontwe mentioned this pull request Sep 19, 2023
Merged
@wmontwe wmontwe requested a review from cketti September 22, 2023 14:43
@wmontwe wmontwe merged commit b5bb166 into main Sep 25, 2023
2 checks passed
@wmontwe wmontwe deleted the add_account_loader branch September 25, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cketti cketti cketti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wmontwe @cketti