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

[CORL-3179]: Create notification on previously rejected comment being approved if DSA is enabled #4671

Merged

Conversation

kabeaty
Copy link
Contributor

@kabeaty kabeaty commented Sep 30, 2024

What does this PR do?

If DSA is enabled, then with these changes, we now create a notification if a comment was previously rejected (whether automatically for including a banned word or by a moderator) and then is approved.

If DSA is not enabled, then there are no changes; no notification is created when a comment is rejected, and there is also no subsequent notification created if it is later approved after rejection.

These changes will impact:

  • commenters
  • moderators
  • admins
  • developers

What changes to the GraphQL/Database Schema does this PR introduce?

This adds a new type to the NOTIFICATION_TYPE enum of PREVIOUSLY_REJECTED_COMMENT_APPROVED.

Does this PR introduce any new environment variables or feature flags?

no

If any indexes were added, were they added to INDEXES.md?

n/a

How do I test this PR?

You can test out these changes by:

  • Enabling DSA
  • As User A, creating a comment with a banned word in it (or creating a comment and then rejecting it); see that it is rejected and gets a notification about this
  • As a User B (with role moderator/admin), approving the rejected comment
  • See that User A has a notification that the comment was previously rejected but has been restored to the page

Then, also:

  • Disabling DSA
  • Repeating the other steps above, but observing that no notifications are created for rejection/approval

Were any tests migrated to React Testing Library?

How do we deploy this PR?

Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for gallant-galileo-14878c canceled.

Name Link
🔨 Latest commit 4011d81
🔍 Latest deploy log https://app.netlify.com/sites/gallant-galileo-14878c/deploys/66fd91dc16c2ac00082fbd91

return getMessage(
bundles,
"notifications-yourPreviouslyRejectedCommentHasBeenApproved",
"Your comment was previously rejected. It has now been restored to the page."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"restored to the page"

Have we checked this copy with Andrew?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what was included in the issue. @losowsky can you confirm that this is the copy we want for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Nick is right, let's change it to "Your comment was previously rejected. It has now been approved."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated...thank you both!

Comment on lines 110 to 111
if (tenant.dsa?.enabled) {
if (previousComment?.status === GQLCOMMENT_STATUS.REJECTED) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am I missing something on the nested if's here? Can it be simplified to the following?

if (tenant.dsa?.enabled && previousComment?.status === GQLCOMMENT_STATUS.REJECTED) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, it can be...updating! thanks

Copy link
Contributor

@nick-funk nick-funk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, only thought is to confirm the copy with Andrew. Could just be me but I don't think we use "page" in our terminology when referring to the comment stream so it stuck out to me.

If I'm wrong and that was the expected copy, am cool with it, feel free to merge!

@tessalt tessalt merged commit d80f5de into develop Oct 3, 2024
6 checks passed
@tessalt tessalt deleted the feat/CORL-3179-dsa-rejected-comments-approved-update branch October 3, 2024 12:22
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.

4 participants