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

Remove concept of main accounts #989

Merged
merged 30 commits into from
Sep 19, 2024
Merged

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Aug 20, 2024

Purpose

Right now a LocalAddressBook still holds a reference to it's main account, but this is obsolete. We already have a relation between an address book account and the main account it belongs to via the corresponding database collection:

"main" account ↔ DB collection ↔ address book ↔ address book account

In the future we want to rely on system accounts as little as possible. Therefore this PR removes the direct relation between an address book account to its "main" account where possible.

Short description

  • Acquire account settings via address book account - Adddress book settings are still stored in the settings of the main account.
  • Create address book account name only from collection info - Although it might be more userfriendly, we don't need the name of the main account here and instead of a hash we can use the collection ID for uniqueness.
  • Remove the "mainAccount" property from LocalAddressBook
  • Stop storing main account name and type in address book account settings
  • Don't allow address book accounts to get settings, must be caldav account - which stores all the settings anyways

Note: We can probably not use getAccountsByTypeAndFeatures to query for an account belonging to a specific db collection. That's because I think we can not set those features ourselves - that is to add the collection ID. Also addAccountExplicitly which we want to user over the normal addAccount, does not seem to have the option to "require account features".

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@sunkup sunkup linked an issue Aug 20, 2024 that may be closed by this pull request
@sunkup sunkup added the refactoring Internal improvement of existing functions label Aug 20, 2024
@sunkup sunkup self-assigned this Aug 20, 2024
@sunkup sunkup force-pushed the 878-remove-concept-of-main-accounts branch 5 times, most recently from 5ab7c47 to b5f196e Compare August 22, 2024 11:10
@sunkup sunkup marked this pull request as ready for review August 27, 2024 12:54
@sunkup sunkup force-pushed the 878-remove-concept-of-main-accounts branch from b5f196e to a118521 Compare September 9, 2024 09:50
@rfc2822 rfc2822 force-pushed the 878-remove-concept-of-main-accounts branch from 9ae52e2 to 8bb9a0b Compare September 13, 2024 09:55
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! We can do it even without bitfireAT/davx5#603.

A few thoughts. Haven't tried in detail but I think it should work.

We probably should also have a talk about this, but maybe you can already have a look.

@sunkup sunkup force-pushed the 878-remove-concept-of-main-accounts branch 4 times, most recently from 958bda8 to 39e4390 Compare September 16, 2024 09:09
@sunkup sunkup force-pushed the 878-remove-concept-of-main-accounts branch from 39e4390 to 67d0409 Compare September 16, 2024 09:43
@rfc2822 rfc2822 force-pushed the 878-remove-concept-of-main-accounts branch from 5ebdad9 to 5c31a8a Compare September 17, 2024 14:33
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the changes and requested changes.

Copy link
Member Author

@sunkup sunkup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a lot. But I think it's actually coming along rather nicely now 👍

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it should be fine now. Can you do a final check? Then I can also have a last look and merge.

@sunkup
Copy link
Member Author

sunkup commented Sep 19, 2024

I hope it should be fine now. Can you do a final check? Then I can also have a last look and merge.

I think we're good. I played around with a bit more as well and it seems to hold up 👍

@sunkup sunkup requested a review from rfc2822 September 19, 2024 12:32
@rfc2822 rfc2822 merged commit b6ceaa7 into main-ose Sep 19, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 878-remove-concept-of-main-accounts branch September 19, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove concept of main accounts
2 participants