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(Core) : Remove account from db and save new one #3221

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

connorivy
Copy link
Contributor

Description & motivation

Because the account id is changing, we no longer need to update the object. Instead, we delete the old object from the db and then save a new one.

Changes:

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

Copy link
Contributor

@BovineOx BovineOx left a comment

Choose a reason for hiding this comment

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

I think it is fine but would like @AlanRynne or @JR-Morgan to cast an eye before merging through

s_accountStorage.UpdateObject(account.id!, JsonConvert.SerializeObject(account));
RemoveAccount(id);
s_accountStorage.SaveObject(account.id, JsonConvert.SerializeObject(account));
await s_accountStorage.WriteComplete().ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine, just unsure about this change to async and why WriteComplete() is necessary, seems a bit janky to me, but that's probably more about the storage?

@BovineOx BovineOx removed the request for review from AlanRynne March 13, 2024 08:45
@JR-Morgan JR-Morgan merged commit ec1191f into main Mar 13, 2024
32 checks passed
@JR-Morgan JR-Morgan deleted the fix-upgrade-account-flow branch March 13, 2024 11:40
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.

None yet

3 participants