Implement archiveEntity GQL mutation and BP hook
#1518
Conversation
github-actions
bot
added
area/apps > hash*
Codecov Report
@@ Coverage Diff @@
## main #1518 +/- ##
==========================================
- Coverage 55.81% 55.79% -0.02%
==========================================
Files 218 218
Lines 14224 14229 +5
Branches 376 376
==========================================
Hits 7939 7939
- Misses 6280 6285 +5
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
π£ Weβre building smart automated test selection to slash your CI/CD build times. Learn more |
|
This pull request introduces 1 alert when merging a44b0a8 into 387317f - view on LGTM.com new alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down β» completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine βοΈ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
|
|
||
| await entityModel.archive(graphApi, { actorId: userModel.getEntityUuid() }); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
What does it mean to return a boolean here? I assume we're throwing an error on the failure case (as I don't see a return false) so would we perhaps want to return null or something if that's even possible?
There was a problem hiding this comment.
Yes we are throwing an error. GraphQL does not have a void type so this seemed like the easiest thing to do. GraphQL also doesn't seem to support returning null as you suggest. It would be possible to introduce a scalar called Void if you feel strongly about not using Boolean.
There was a problem hiding this comment.
Oh right it doesn't support null, remember talking about this a long time ago. Boolean seems fine for now
|
|
||
| const { entityId } = data; | ||
|
|
||
| await archiveEntityFn({ variables: { entityId } }); |
There was a problem hiding this comment.
Do you not need to use the return value here?
There was a problem hiding this comment.
No, because I've made the return-type of the BP hook Promise<void>
There was a problem hiding this comment.
We seem to throw errors when data is nullish in other hooks so I feel like we need to check it's true here?
There was a problem hiding this comment.
Why is data nullable in other hooks?
There was a problem hiding this comment.
I feel like the error throwing when data is falsy is a consequence of an incorrect return-type, so not something that should influence the decision of which return-type to use here.
There was a problem hiding this comment.
It doesn't seem to be our configuration, but actually hardcoded in the type from Apollo
There was a problem hiding this comment.
I think data can be undefined in apollo, because they return errors rather than throwing them (see the error field in your screenshot). I now see that this is what we are doing for the BP hooks as well:
However, I still don't think returning data: undefined should be treated as an error state, instead if errors is a non-empty array it should indicate that an error has occurred in the archiveEntity hook.
There was a problem hiding this comment.
I've looked at the current graph service definition, and it seems we should be returning a boolean instead so have changed it to that in 6f3978a
|
This pull request introduces 1 alert when merging 6f3978a into 69dc78c - view on LGTM.com new alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down β» completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine βοΈ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
vilkinsons
added
type/eng > frontend
π What is the purpose of this PR?
This PR introduces an
archiveEntityGQL mutation and BP hook.π Related links
π What does this change?
See commits.
π Does this require a change to the docs?
No.
π‘ What tests cover this?
None.
β How to test this?
yarn devhttp://localhost:3000/entity-editorπΉ Demo
Screen.Recording.2022-11-28.at.12.43.57.mov