-
Notifications
You must be signed in to change notification settings - Fork 356
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
[CORL-2891]: fix edit child reply to edited parent not updating #4334
[CORL-2891]: fix edit child reply to edited parent not updating #4334
Conversation
✅ Deploy Preview for gallant-galileo-14878c canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to need to test this, but have compiled my thoughts here.
if (input.parentRevisionID) { | ||
const revision = getLatestRevision(parent); | ||
if (revision.id !== input.parentRevisionID) { | ||
throw new CommentRevisionNotFoundError( | ||
parent.id, | ||
input.parentRevisionID | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It think the reason this wasn't being checked here is that it is possible for a comment to come in from a stale page (or conversation view) that has old revision ID's. If we do this, they will likely get errors when replying to a stale comment revision.
I recently had a person making a long reply to one of my comments on HN and I edited it while they were writing their reply, so I at leas know this can happen in the wild on other platforms.
In fast paced comment sections, I would imagine this happens a bit more frequently as well, where people are editing their comment simultaneously while others reply to them.
I'm not sure if we want to put this in here?
// Check to see that the most recent revision matches the one we just replied | ||
// to. | ||
const revision = getLatestRevision(parent); | ||
if (revision.id !== input.parentRevisionID) { | ||
throw new CommentRevisionNotFoundError(parent.id, input.parentRevisionID); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, now I'm tad bit confused, we were always checking against latest revision this whole time?
Does the check added to createComment
matter then? Or was this only being called for retrieveParent
which wasn't being used in createComment
?
Hmmm... I'm going to have to test this to see what's going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating with what we discussed offline; removing this check altogether since we don't think it's necessary to check if replies are being made to stale comments or if replies to stale comment versions are being edited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad we got that errant check out of the way, approved!
What does this PR do?
These changes make it so that we only check that the latest parent comment revision matches up with the parent revision id input when creating a comment reply. Previously, we were also checking whenever retrieving a parent comment (including when a comment is edited).
This makes it so that child replies, to a parent comment that is edited after the reply has been created, can still be edited themselves.
These changes will impact:
What changes to the GraphQL/Database Schema does this PR introduce?
none
Does this PR introduce any new environment variables or feature flags?
none
If any indexes were added, were they added to
INDEXES.md
?n/a
How do I test this PR?
Create a comment. Create a reply to the comment. Edit the parent comment. Edit the reply comment. See that everything is edited/updated as expected and you get no errors.
Where any tests migrated to React Testing Library?
How do we deploy this PR?