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

Breadcrumbs issue #213 fix #314

Merged
merged 9 commits into from
Jul 4, 2023

Conversation

andrew-jp
Copy link
Contributor

@andrew-jp andrew-jp commented Jun 12, 2023

Issue #213

This is a working fix for the breadcrumbs issue. It copies/updates/sets the updated areas tokens, and then calls a recursive function on each of the areas children (in this case just Mongoose object id's). The recursive function looks up the object id (queries the db), and if it has path tokens it updates the index (a depth counter is passed into each recursive call to track this index) where the name change occurred. It then sets the pathTokens in the object and saves. It will throw if the save fails or if the query for the child object returns null.

Performance varied a little in my testing. To update a large area (Central Wasatch in Utah for example) it needs to update a large tree structure, with all the associated query and save ops. This initially took about 7.5s, but in my recent testing only about 5s. I don't how much this is machine dependent, but I imagine this solution isn't performant regardless. I'll try to improve the performance where I can.

Any input on error handling and performance is welcome.

@vnugent vnugent requested a review from zichongkao June 12, 2023 21:45
src/model/MutableAreaDataSource.ts Outdated Show resolved Hide resolved
@vnugent
Copy link
Contributor

vnugent commented Jun 14, 2023

@andrew-jp I checked out the PR locally and did a quick smoke test. It updated all nested child areas as expected and took less than 3s on my old Macbook. Nice work!

I notice the change history code generates a lot of warnings, "History Id not found. Ignore change." I think we will also need to pass down the history Id so that all recursive updates are also recorded in the same changeset. I'll dig a bit deeper and let you know.

Meanwhile can you look into adding a few unit tests? https://github.com/OpenBeta/openbeta-graphql/blob/develop/src/model/__tests__/updateAreas.ts

@andrew-jp
Copy link
Contributor Author

andrew-jp commented Jun 14, 2023

I added a basic test that checks a small tree's pathTokens before/after the update. It matches them exactly, ie: ['Japan', 'Test Change', 'B1', 'C1']. I'd like to have a test that verifies a total rollback if we get an error in the recursion, but I don't know how to go about creating that error. I'll consider more cases tomorrow.

@vnugent
Copy link
Contributor

vnugent commented Jun 15, 2023

Not 100% sure if it's going to work. You can try mocking the implementation of updatePathTokens()

On the first invocation return the default implementation. On the 2nd (or 3rd) mock the implementation with a function that simply throws an exception.

https://stackoverflow.com/questions/61450440/jest-spyon-to-mock-implementation-only-on-second-call-and-the-third-call
https://stackoverflow.com/questions/62225496/modify-mockimplementation-results-in-spyon-of-jest

@vnugent
Copy link
Contributor

vnugent commented Jul 4, 2023

We can add more unit tests in a follow up PR

@vnugent vnugent merged commit ccc3b6a into OpenBeta:develop Jul 4, 2023
1 check passed
@musoke
Copy link
Contributor

musoke commented Jul 14, 2023

I didn't notice this got merged until I saw it in action on openbeta.io - nicely done @andrew-jp!

We can add more unit tests in a follow up PR
Do we need an issue this or are they low priority?

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