[HOLD for payment 2023-09-21] [$1000] Web - Flawed Attachment Visibility: Flagged “hidden” Attachments still visible from attachment modal

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. From User A, Initiate a chat with User B
2. From user A, send some attachments (atleast 2 or 3)
3. From user B, flag any attachment with moderation as “hidden” like bullying
4. From user A, click on an attachment to open attachment modal
5. Click on the left or right button if exist to see all attachment
6. Verify that the attachment that is flagged above and supposed to be “hidden” still appears

Expected Result:

When an attachment is flagged and marked as not visible, users should be restricted from seeing it

Actual Result:

Both Users were able to see it

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.40-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Upload-.flagged.attachment.visi.mp4

Recording.3633.mp4

Expensify/Expensify Issue URL:
Issue reported by: @avi-shek-jha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689289028489799

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014153535e195ba609
• Upwork Job ID: 1681657037569265664
• Last Price Increase: 2023-08-08
• Automatic offers:
• fedirjh | Reviewer | 26121902
• bernhardoj | Contributor | 26121905
• avi-shek-jha | Reporter | 26121906

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We can still clearly see the attachment on the carousel that is hidden in chat because of flagging. I wouldn't say this is a bug, but rather a polish.

What is the root cause of that problem?

It's simply because we don't hide the attachment on the carousel.

What changes do you think we should make in order to solve the problem?

As the image is hidden on the chat, we can also hide it on the carousel. Here are the steps:

1. Check if the report action should be hidden and then pass it to the HTML (here)
   

   App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

   Lines 37 to 42 in 2ce4328

   	_.forEach(actions, (action, key) => {
   	if (!ReportActionsUtils.shouldReportActionBeVisible(action, key)) {
   	return;
   	}
   	htmlParser.write(_.get(action, ['message', 0, 'html']));
   	});

const decision = _.get(action, ['message', 0, 'moderationDecision', 'decision'], '');
const isHidden = decision === CONST.MODERATION.MODERATOR_DECISION_PENDING_HIDE || decision === CONST.MODERATION.MODERATOR_DECISION_HIDDEN;
const html = _.get(action, ['message', 0, 'html'], '').replace('/>', `data-hidden="${isHidden}" />`);
htmlParser.write(html)

2. On parsing the HTML, add isHidden attribute to the attachment object (here)
   

   App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

   Lines 29 to 33 in 2ce4328

   	attachments.unshift({
   	source: tryResolveUrlFromApiRoot(expensifySource || attribs.src),
   	isAuthTokenRequired: Boolean(expensifySource),
   	file: {name: attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE]},
   	});

   
   isHidden: attribs['data-hidden'] === "true",
3. Create a new component (CarouselItem) to handle the hide logic of the attachment

the code

import React, {useState, useEffect} from 'react'
import CONST from '../../../CONST';
import styles from '../../../styles/styles';
import useLocalize from '../../../hooks/useLocalize';
import {PressableWithoutFeedback} from '../../Pressable';
import Text from '../../Text';
import Button from '../../Button';
import AttachmentView from '../AttachmentView';

function CarouselItem({item, isFocused, onPress}) {
    const {translate} = useLocalize();
    const [isHidden, setIsHidden] = useState(item.isHidden);

    useEffect(() => {
        if (!isFocused) setIsHidden(item.isHidden);
    }, [isFocused]);

    if (isHidden) {
        return (
            <PressableWithoutFeedback
                style={[styles.w100, styles.h100, styles.alignItemsCenter, styles.justifyContentCenter, {paddingHorizontal: 58}]}
                onPress={onPress}
                accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
                accessibilityLabel={item.file.name || translate('attachmentView.unknownFilename')}
            >
                <Text style={[styles.textLabelSupporting, styles.textAlignCenter, styles.lh20]}>{translate('moderation.flaggedContent')}</Text>
                <Button
                    small
                    style={[styles.mt2]}
                    onPress={() => setIsHidden(false)}
                >
                    <Text
                        style={styles.buttonSmallText}
                        selectable={false}
                    >
                        {translate('moderation.revealMessage')}
                    </Text>
                </Button>
            </PressableWithoutFeedback>
        );
    }

    return (
        <AttachmentView
            source={item.source}
            file={item.file}
            isAuthTokenRequired={item.isAuthTokenRequired}
            isFocused={isFocused}
            onPress={onPress}
            isUsedInCarousel
        />
    );
}

export default CarouselItem;

4. Replace the renderItem of the carousel from AttachmentView to CarouselItem

Result:

Screen.Recording.2023-07-15.at.13.38.23.1.mov

[allroundexperts]

Hi @dukenv0307,
This seems like a regression from #21625?

[dukenv0307]

@allroundexperts I don't think it's a regression because in the previous bug we only hide the image that has moderation decision is pendingRemove. And this issue is not a case of this.

[melvin-bot]

@JmillsExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[JmillsExpensify]

Triaging it.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014153535e195ba609

[melvin-bot]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)

[fedirjh]

I think we should put this issue on hold until this PR is merged :

• [HOLD for payment 2023-08-08] [$1000] Attachment preview does not update dynamically #21334

[JmillsExpensify]

Good point. I agree with that.

[JmillsExpensify]

Looks like we should now re-test and take this issue off hold.

[fedirjh]

@JmillsExpensify I just retested it, looks like the bug was not fixed. Let's take it off hold.

---

cc @bernhardoj could you please update your proposal, Looks like we did a recent refactor to the AttachmentCarousel and the code you mentioned seems to be updated/ removed ?

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @bernhardoj got assigned: 2023-08-14 23:05:25 Z
• when the PR got merged: 2023-09-12 17:35:46 UTC
• days elapsed: 20

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Hide flagged attachment in attachment carousel #24564

If no regressions arise, payment will be issued on 2023-09-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @fedirjh requires payment offer (Reviewer)
• @bernhardoj requires payment offer (Contributor)
• @avi-shek-jha requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[fedirjh]

BugZero Checklist:

• Link to the PR: N/A This is not a regression
• Link to comment: N/A
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: ✅

---

Regression Test Proposal

1. As User A, open the app and navigate to User B's chat.
2. Send an attachment to User B.
3. As User B, flag the attachment as "Intimidation/Bullying."
4. As User A, Open the attachments carousel and navigate to the flagged attachment.
5. Verify that the attachment is hidden
6. Verify you see a centered text ("This message has been flagged...") with the button ("Reveal message") similar to hidden comments.
7. Press the "Reveal message" button.
8. Verify that you can now see the attachment.
9. Press the "Hidden message" button.
10. Verify that the attachment is hidden.

• Do we agree 👍 or 👎

[JmillsExpensify]

@fedirjh Thanks for kicking off the BZ checklist. That means it's time for me to circle back with a payment summary:

• Issue reporter: @avi-shek-jha $250
• Contributor: @bernhardoj $1,000
• Contributor+: @fedirjh $1,000

[JmillsExpensify]

Offers sent to all contributors.

[avi-shek-jha]

Offers sent to all contributors.

Offer Accepted. Thank You.

[fedirjh]

Offers sent to all contributors.

Seems the offer was expired for me.

[bernhardoj]

@JmillsExpensify I get 2 offers. I accepted one of them.

[melvin-bot]

@JmillsExpensify, @amyevans, @fedirjh, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@JmillsExpensify, @amyevans, @fedirjh, @bernhardoj Still overdue 6 days?! Let's take care of this!

[amyevans]

Friendly bump @JmillsExpensify on #22915 (comment)

[JmillsExpensify]

Paid out issue reporter and contributor. Just send another offer to you @fedirjh.

[fedirjh]

@JmillsExpensify Accepted, thank you.

[JmillsExpensify]

All paid out! Regression test was also added so I'm closing this issue.