[HOLD for payment 2024-07-26] [Closed & Archived] Extra spacing between last message and archived banner

[srikarparsi]

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

---

Follow up issue from here. More info in this comment.

You can reproduce by replacing this with return false so that the isArchivedRoom check doesn't rely on the fallback (which will be removed in a future PR).

Version Number:
Reproducible in staging?:
Reproducible in production?:
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
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

Action Performed:

Break down in numbered steps

1. Create a workspace
2. Send a message in a workspace chat (admins, announce...)
3. Delete the workspace
4. Navigate back to the workspace chat

Expected Result:

Describe what you think should've happened

There should be the right spacing between the last message and the archived banner in the footer

Actual Result:

Describe what actually happened

There is too much spacing between the last message and the archived banner in the footer

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[devguest07]

Proposal

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

Extra spacing between last message and archived banner

What is the root cause of that problem?

Margin top used as mt4

App/src/pages/home/report/ReportFooter.tsx

Line 171 in b4b376e

isArchivedRoom || isAnonymousUser || !canWriteInReport || (isAdminsOnlyPostingRoom && !isUserPolicyAdmin) ? styles.mt4 : {},

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

Decrease the margin at the top

App/src/pages/home/report/ReportFooter.tsx

Line 171 in b4b376e

isArchivedRoom || isAnonymousUser || !canWriteInReport || (isAdminsOnlyPostingRoom && !isUserPolicyAdmin) ? styles.mt4 : {},

[srikarparsi]

@DylanDylann you can reproduce by replacing this with return false so that the isArchivedRoom check doesn't rely on the fallback (which will be removed in a future PR).

function isArchivedRoom(report: OnyxInputOrEntry<Report>, reportNameValuePairs?: OnyxInputOrEntry<ReportNameValuePairs>): boolean {
    if (reportNameValuePairs) {
        return reportNameValuePairs.private_isArchived;
    }

    return false;
}

[Krishna2323]

Proposal

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

Extra spacing between last message and archived banner

What is the root cause of that problem?

hideComposer is false in ReportActionsList.tsx even when composer is hidden and shouldHideComposer in ReportFooter.tsx is correctly set as true when composer is hidden. In ReportActionsList.tsx we apply padding top when !hideComposer is true and !hideComposer produces true even if the report is archived.

App/src/pages/home/report/ReportActionsList.tsx

Line 601 in dc5d06b

const hideComposer = !ReportUtils.canUserPerformWriteAction(report);

App/src/pages/home/report/ReportFooter.tsx

Line 106 in dc5d06b

const shouldHideComposer = (!ReportUtils.canUserPerformWriteAction(report, reportNameValuePairs) && !shouldShowComposerOptimistically) || isBlockedFromChat;

App/src/pages/home/report/ReportActionsList.tsx

Line 689 in dc5d06b

<Animated.View style={[animatedStyles, styles.flex1, !shouldShowReportRecipientLocalTime && !hideComposer ? styles.pb4 : {}]}>

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

!hideComposer should return false when report is archived, we should make it consistent with shouldHideComposer in ReportFooter.tsx. We can refactor and create a new function to use in ReportActionsList.tsx and ReportFooter.tsx or we can just do same as in ReportFooter.tsx to get the correct value of hideComposer.

    const [isBlockedFromChat] = useOnyx(ONYXKEYS.NVP_BLOCKED_FROM_CHAT, {
        selector: (dateString) => {
            if (!dateString) {
                return false;
            }
            try {
                return new Date(dateString) >= new Date();
            } catch (error) {
                // If the NVP is malformed, we'll assume the user is not blocked from chat. This is not expected, so if it happens we'll log an alert.
                Log.alert(`[${CONST.ERROR.ENSURE_BUGBOT}] Found malformed ${ONYXKEYS.NVP_BLOCKED_FROM_CHAT} nvp`, dateString);
                return false;
            }
        },
    });

    const [isAnonymousUser = false] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.authTokenType === CONST.AUTH_TOKEN_TYPES.ANONYMOUS});
    const shouldShowComposerOptimistically = !isAnonymousUser && ReportUtils.isPublicRoom(report) && !!reportMetadata?.isLoadingInitialReportActions;
    const shouldHideComposer = (!ReportUtils.canUserPerformWriteAction(report, reportNameValuePairs) && !shouldShowComposerOptimistically) || isBlockedFromChat;

reportMetadata and reportNameValuePairs can be passed down to ReportActionsList from ReportScreen or we can obtain that from onyx.

What alternative solutions did you explore? (Optional)

[trjExpensify]

Is @DylanDylann implementing this as a follow-up to his PR?

[srikarparsi]

Yes, that's the goal. It was a bug I found from the PR but thought it would be better to handle as a follow up so that we could merge without conflicts.

[trjExpensify]

Great! @DylanDylann can you raise the PR please?

[DylanDylann]

Yeah, The PR will be ready soon

[trjExpensify]

Did we get a PR up for this?

[DylanDylann]

I am working on it

[DylanDylann]

@srikarparsi After investigating, It seems this bug isn't regression from my PR

The RCA: In #40661, we add reportNameValuePairs argument to canUserPerformWriteAction function but we don't add reportNameValuePairs param when using canUserPerformWriteAction function in some places

My solution: Removing reportNameValuePairs argument in canUserPerformWriteAction function. Getting reportNameValuePairs in canUserPerformWriteAction function directly

[DylanDylann]

@srikarparsi The PR is up #45588

[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 9.0.9-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [NoQA] Remove reportNameValuePairs argument #45588

If no regressions arise, payment will be issued on 2024-07-26. 🎊

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

• @DylanDylann requires payment (Needs manual offer from BZ)

[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:

• [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
• [@DylanDylann] 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:
• [@DylanDylann] 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:
• [@DylanDylann] Determine if we should create a regression test for this bug.
• [@DylanDylann] 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.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[srikarparsi]

The payment for this will be taken care of in this issue since it was brought up in this comment. Does that sound right @DylanDylann?

[DylanDylann]

Yes. Let's close this one