-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Handle Moderated Messages in UI #19476
Conversation
@thesahindia @MariaHCD One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
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.
functionality to actually moderate these messages isn't quite there but this needs to go out soon.
Just wondering, is the backend not set up yet? And if so, why is the frontend going out first?
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.
Not a full review, just a few questions but will take a closer soon on Monday!
return; | ||
} | ||
|
||
// Right now we are only sending the latest moderationDecision to the frontend even though it is an array |
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.
Is this because any previous moderation decisions won't really be used in the frontend?
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.
yep! We originally were just going to send them all because it was simpler for the backend, but this made a little more sense for JS.
@MariaHCD So it's all in process, the actual flagging is set up in the backend but the ability for concierge to make the decisions on it isn't quite yet. Basically we're trying to get this out quickly (for Thursday) so pushing the pieces we can, when we can. Definitely not ideal, of course, but we'll get there! |
// Hide the message if it is being moderated for a higher offense, or is hidden by a moderator | ||
// Removed messages should not be shown anyway and should not need this flow | ||
useEffect(() => { | ||
if (_.isEmpty(props.action.moderationDecisions)) { |
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.
Let's first check that it's an ADDCOMMENT report action and then update to check the message object like we talked about
setIsHidden(true); | ||
} | ||
setModerationDecision(latestDecision.decision); | ||
}, [props.action, moderationDecision]); |
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.
Can we check props.action.message so this doesn't re-render as much? Also it doesn't seem like we need to add a case for moderationDecision. It only changes when the prop.action.message changes, and then it would cause another run of this since the moderationDecision changed
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.
yeah i agree on moderationDecision... but the linter did not haha. I'll see if switching over to the right format for the message object fixes that.
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.
Great - thanks!
@thesahindia Could you please do a code review for this and verify it doesn't break the existing app reportActions? |
// Hide the message if it is being moderated for a higher offense, or is hidden by a moderator | ||
// Removed messages should not be shown anyway and should not need this flow | ||
useEffect(() => { | ||
if (!props.action.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT || _.isEmpty(props.action.message[0].moderationDecisions)) { |
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.
if (!props.action.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT || _.isEmpty(props.action.message[0].moderationDecisions)) { | |
if (props.action.actionName !== CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT || _.isEmpty(props.action.message[0].moderationDecisions)) { |
NAB, it was just a bit confusing to read at first.
|
||
// Right now we are only sending the latest moderationDecision to the frontend even though it is an array | ||
const latestDecision = props.action.message[0].moderationDecisions[0]; | ||
if (latestDecision.decision === CONST.MODERATION.MODERATOR_DECISION_PENDING_HIDE || latestDecision.decision === CONST.MODERATION.MODERATOR_DECISION_HIDDEN) { |
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.
if (latestDecision.decision === CONST.MODERATION.MODERATOR_DECISION_PENDING_HIDE || latestDecision.decision === CONST.MODERATION.MODERATOR_DECISION_HIDDEN) { | |
if (_.contains([CONST.MODERATION.MODERATOR_DECISION_PENDING_HIDE, CONST.MODERATION.MODERATOR_DECISION_HIDDEN], latestDecision.decision)) { |
NAB
Over to you, @thesahindia! :) |
Will get to this in a few minutes. |
Reviewer Checklist
Screenshots/Videos |
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.
Works well!
I was wondering whether we should also disable some options (i.e reaction button) when we hide the message?
Also I noticed that when we open the thread for a hidden message, the message appears in the chat header, so should we hide the chat header text as well?
I'm going to go ahead and merge this (for expendiency) and create a follow up PR to deal with the callouts that @thesahindia made (and the NABs from @MariaHCD ) |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/dangrous in version: 1.3.21-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.21-2 🚀
|
style={styles.buttonSmallText} | ||
selectable={false} | ||
> | ||
{isHidden ? 'Reveal message' : 'Hide message'} |
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.
You forgot to translate this which caused a regression #20090
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.
Yep, saw that one when it happened... I changed it in one of the places it appears but not the other one. Thanks!
setIsHidden(true); | ||
} | ||
setModerationDecision(latestDecision.decision); | ||
}, [props.action.message, props.action.actionName]); |
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.
✋ Coming from #20810
We're interested in the value of moderationDecisions[0].decision
. We should change the dependency of the effect.
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.
ah nice!
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.
makes sense! we are also eventually switching this to only ever have one moderation decision, so we won't need this, but the fix in #20810 is great for now
@@ -105,6 +106,8 @@ const defaultProps = { | |||
|
|||
function ReportActionItem(props) { | |||
const [isContextMenuActive, setIsContextMenuActive] = useState(ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)); | |||
const [isHidden, setIsHidden] = useState(false); | |||
const [moderationDecision, setModerationDecision] = useState(CONST.MODERATION.MODERATOR_DECISION_APPROVED); |
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.
Hi, we missed a small offline edge case in this PR - #20946
On deleting a hidden message, the reveal message button should have been removed too.
More details here #20946 (comment)
// Right now we are only sending the latest moderationDecision to the frontend even though it is an array | ||
const latestDecision = props.action.message[0].moderationDecisions[0]; | ||
if (latestDecision.decision === CONST.MODERATION.MODERATOR_DECISION_PENDING_HIDE || latestDecision.decision === CONST.MODERATION.MODERATOR_DECISION_HIDDEN) { | ||
setIsHidden(true); |
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.
We fail to set a hidden false case here which causes a regression #22024 , For more details refer this proposal
Details
This SHOULD work but right now you have to manually adjust the state to make the messages "flagged" to test it because we don't have
moderationDecisions
yet.IOS was broken for me on dev - both on
main
and in this branch, so it's unrelated - so I was unable to get a screenshot. Please check this platform extra carefully. (I tried for like 30 min to fix it but haven't yet.)Fixed Issues
$ #18509
Tests
For now, please edit this line to be default true
and this line to be default the status you're attempting to test. (functionality to actually moderate these messages isn't quite there but this needs to go out soon.
The effects should be:
These will be sent to the frontend via the
moderatorDecision
in the message array, it will look like:There will only be one decision max.
Offline tests
Same as online
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android