Mutables for the Tree 🎄 + clean up TreeItem observers and mutables properly #6032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Ensure that the "Saving" modal dialog is closed - Notify user of the error, and also print to console to catch in e2e
- If create fails due to a conflict or otherwise, and the user immediately tries to "Create" again, default the selector tree's selected item to the "mine" folder (which we know exists).
- Only use mutables and observers if NOT a SelectorTree - Properly clean up observers and mutables when a parent item is removed from the tree
ozyx
changed the title
Codecov Report
@@ Coverage Diff @@
## master #6032 +/- ##
==========================================
- Coverage 55.07% 54.99% -0.09%
==========================================
Files 600 600
Lines 23696 23748 +52
Branches 2109 2112 +3
==========================================
+ Hits 13051 13060 +9
- Misses 10055 10098 +43
Partials 590 590
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Error notification on 'My Items' folder missing was removed, so don't check for it
ozyx
added
pr:e2e:couchdb
ozyx
added
pr:e2e:couchdb
ozyx
added
pr:e2e:couchdb
unlikelyzero
reviewed
Dec 2, 2022
| const page2 = await page.context().newPage(); | ||
|
|
||
| await Promise.all([ | ||
| page.goto('./', { waitUntil: 'networkidle' }), |
| createDomainObjectWithDefaults(page2, { | ||
| type: 'Clock' | ||
| }) | ||
| ]); |
| ]); | ||
|
|
||
| // Start logging console errors from this point on | ||
| let errors = []; |
There was a problem hiding this comment.
Are we waiting until now because of the mine error we couldn't solve last sprint
There was a problem hiding this comment.
Waiting until now so that we don't capture the expected 409 error from couchdb
|
Testing creation of objects after a conflict appears to work: Screen.Recording.2022-12-05.at.7.26.04.PM.mov |
jvigliotta
reviewed
Dec 5, 2022
| if (checkItem.navigationPath !== path | ||
| && checkItem.navigationPath.includes(path)) { | ||
| this.destroyObserverByPath(checkItem.navigationPath); | ||
| this.destroyMutableByPath(checkItem.navigationPath); |
jvigliotta
reviewed
Dec 5, 2022
akhenry
requested changes
Dec 16, 2022
- Remove check for typeof 'function' to not hide any potential coding errors
akhenry
requested changes
Dec 17, 2022
OK, I think I've got a fix for this. We rethrow the error on a conflicted save, allowing |
akhenry
approved these changes
Dec 20, 2022
jvigliotta
reviewed
Dec 20, 2022
| @@ -234,7 +234,8 @@ class CouchObjectProvider { | |||
| #handleResponseCode(status, json, fetchOptions) { | |||
| this.indicator.setIndicatorToState(this.#statusCodeToIndicatorState(status)); | |||
| if (status === CouchObjectProvider.HTTP_CONFLICT) { | |||
| throw new this.openmct.objects.errors.Conflict(`Conflict persisting ${fetchOptions.body.name}`); | |||
| const objectName = JSON.parse(fetchOptions.body)?.model?.name; | |||
There was a problem hiding this comment.
YES. Thank you for doing doing this. I was running into this one as well.
jvigliotta
approved these changes
Dec 20, 2022
ozyx
added a commit
that referenced
this pull request
Dec 29, 2022
…vers and mutables properly * fix: refresh object after conflict error * fix: recover from error thrown during create - Ensure that the "Saving" modal dialog is closed - Notify user of the error, and also print to console to catch in e2e * fix: default selector tree item to 'mine' folder - If create fails due to a conflict or otherwise, and the user immediately tries to "Create" again, default the selector tree's selected item to the "mine" folder (which we know exists). * fix: don't listen to composition if Selector Tree * refactor: remove dead code * fix: use MutableDomainObjects in the tree - Only use mutables and observers if NOT a SelectorTree - Properly clean up observers and mutables when a parent item is removed from the tree * test: verify conflicts don't break object creation * test: verify dialog closes and object is created * refactor(e2e): update test - Error notification on 'My Items' folder missing was removed, so don't check for it * test: increase timeout * refactor(e2e): use Promise.any() * refactor(e2e): use Promise instead of polling * test: add 2p annotation * test: use `waitForRequest` instead of promise - tidy up test, add comments describing our pattern * docs(e2e): add best practices for network tests * refactor(e2e): avoid using Promise.any * fix: de-reactify observer and mutable maps * fix: destroy by path on treeItem close * fix: don't refresh for synchronized objects * docs: fix a typo 🔥 * fix: remove existing mutable before adding * fix: fail fast if these aren't functions - Remove check for typeof 'function' to not hide any potential coding errors * fix: walk up navigationPath if item not found * chore: fix lint errors * fix: parse conflicted object name correctly * fix: re-throw conflict error * fix: Cancel edit mode on conflict
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5982 #5975
Describe your changes:
All Submissions:
Author Checklist
Reviewer Checklist