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

Fix old address book accounts not being deleted #1039

Merged
merged 15 commits into from
Oct 9, 2024

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Sep 23, 2024

Depends on #1047 so that the migration is not run in main thread


Purpose

#989 has renamed address book accounts. The introduced migration creates new address books and fails at deleting the old ones. Now old orphan ones are lying around and even caused exceptions.

  • An exception should not have been thrown. We should simply log a warning instead.

  • Orphaned address book accounts should of course not exist in the first place and be deleted by the AccountsCleanupWorker in case they do exist.

  • In the migration: Instead of syncing to create new address book accounts and then deleting the old ones with the AccountsCleanupworker we should actually rename the existing address book accounts before a first sync happens in the account settings migration.

Short description

  • Log warning in sync adapter when address book account does not have an associated collection/service/account instead of throwing an (uncought) exception.
  • Fix AccountsCleanupWorker (was not working)
  • Rename the address book accounts before a first sync happens in an account settings migration.

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 Sep 23, 2024 that may be closed by this pull request
@rfc2822 rfc2822 added the bug Something isn't working label Sep 28, 2024
@github-actions github-actions bot removed the dependent label Oct 3, 2024
Copy link

github-actions bot commented Oct 3, 2024

This PR/issue depends on:

1 similar comment
Copy link

github-actions bot commented Oct 3, 2024

This PR/issue depends on:

@sunkup sunkup force-pushed the 1037-crashes-after-updating-to-443-beta1 branch from e64acf0 to e95e58e Compare October 7, 2024 09:41
@sunkup sunkup self-assigned this Oct 7, 2024
@sunkup sunkup requested a review from ArnyminerZ October 7, 2024 13:15
@sunkup sunkup marked this pull request as ready for review October 7, 2024 13:17
Copy link
Member

@ArnyminerZ ArnyminerZ left a comment

Choose a reason for hiding this comment

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

Looks good to me, let's see if @rfc2822 has anything to say

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 have 😁 First bunch (will have a look at AccountsCleanupWorker and AccountSettings afterwards).

I have also merged a few comments / KDoc in #1059, please update from main-ose branch accordingly.

@sunkup sunkup force-pushed the 1037-crashes-after-updating-to-443-beta1 branch from 38a6042 to c25b2d9 Compare October 8, 2024 10:24
@sunkup sunkup force-pushed the 1037-crashes-after-updating-to-443-beta1 branch from c25b2d9 to 6616c57 Compare October 8, 2024 10:25
@sunkup sunkup requested a review from rfc2822 October 8, 2024 10:38
@rfc2822 rfc2822 force-pushed the 1037-crashes-after-updating-to-443-beta1 branch from b3f8077 to f4a857e Compare October 9, 2024 10:09
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.

Looks good, hopefully we didn't overlook something.

@rfc2822 rfc2822 merged commit 26a670c into main-ose Oct 9, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 1037-crashes-after-updating-to-443-beta1 branch October 9, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes after updating to 4.4.3-beta.1
3 participants