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

[HOLD sqllite deployed to production] Add functionality for multiRemove() #219

Closed
wants to merge 10 commits into from

Conversation

tgolen
Copy link
Collaborator

@tgolen tgolen commented Dec 28, 2022

Related Issues

Fixes https://github.com/Expensify/Expensify/issues/251798

Automated Tests

I added some, but it's difficult to test this fully with both data providers. I tested this by adding it to App and testing that signin/singout worked (especially on a large account)

Linked PRs

Expensify/App#13886

@tgolen tgolen changed the title Tgolen implement multiremove Add functionality for multiRemove() Dec 29, 2022
@tgolen tgolen changed the title Add functionality for multiRemove() [HOLD sqllite] Add functionality for multiRemove() Dec 29, 2022
@tgolen tgolen marked this pull request as ready for review December 29, 2022 18:41
@tgolen tgolen requested a review from a team as a code owner December 29, 2022 18:41
@melvin-bot melvin-bot bot requested review from madmax330 and removed request for a team December 29, 2022 18:42
@tgolen tgolen changed the title [HOLD sqllite] Add functionality for multiRemove() [HOLD sqllite deployed to production] Add functionality for multiRemove() Dec 29, 2022
lib/storage/providers/AsyncStorage.js Outdated Show resolved Hide resolved
lib/storage/providers/LocalForage.js Outdated Show resolved Hide resolved
lib/storage/providers/LocalForage.js Show resolved Hide resolved
@yuwenmemon
Copy link
Contributor

Should we also add a basic test for the multiRemove functionality itself?

@tgolen
Copy link
Collaborator Author

tgolen commented Jan 3, 2023

Thanks @yuwenmemon for the review!

I did add a couple of basic tests for multiRemove() in this PR, but were you thinking about more? I was discussing some other ways to test this in this thread which basically ended with us feeling like the tests written here are enough.

tgolen and others added 3 commits January 3, 2023 15:40
Co-authored-by: Yuwen Memon <yuwen@expensify.com>
Co-authored-by: Yuwen Memon <yuwen@expensify.com>
Co-authored-by: Yuwen Memon <yuwen@expensify.com>
@yuwenmemon
Copy link
Contributor

yuwenmemon commented Jan 3, 2023

I think that's fair. My only thought was maybe adding tests similar to what we have for clear() and multiMerge(). But looks like both of those are meant to check for regressions for some more esoteric cache behavior.

Also sorry for screwing you on the line indentation with my suggestions 😅

@tgolen
Copy link
Collaborator Author

tgolen commented Jan 4, 2023

OK, I've actually been working on this second PR that fixes Expensify/App#13884 and it looks like we aren't going to need multiRemove() after all. So, I think I am just going to close this and continue with the other PR

@tgolen tgolen closed this Jan 4, 2023
@tgolen tgolen deleted the tgolen-implement-multiremove branch January 4, 2023 01:45
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.

2 participants