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

[HOLD for payment 2023-09-20] [HOLD for payment 2023-09-18][$1000] Inconsistency: Different grayness levels on parent and child message #24263

Closed
1 of 6 tasks
kavimuru opened this issue Aug 8, 2023 · 77 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Aug 8, 2023

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. Be Offline
  2. Go to any chat
  3. Send a message
  4. Go reply in thread
  5. send a reply
    Notice the level of grayness on the Parent and child message

Expected Result:

Same grayness levels

Actual Result:

Different grayness levels

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 / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.51-0
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

Test99.Graynesslevel-1.mp4

Snip - (9) New Expensify - Google Chrome

Expensify/Expensify Issue URL:
Issue reported by: @daveSeife
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690943139195889

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0192daee63c42d4dbd
  • Upwork Job ID: 1689013073676578816
  • Last Price Increase: 2023-08-15
  • Automatic offers:
    • ginsuma | Contributor | 26258577
    • daveSeife | Reporter | 26258578
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

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

@tienifr
Copy link
Contributor

tienifr commented Aug 8, 2023

Proposal

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

Inconsistency: Different grayness levels on parent and child message

What is the root cause of that problem?

<OfflineWithFeedback
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}
errorRowStyles={[styles.ml10, styles.mr2]}
onClose={() => Report.navigateToConciergeChatAndDeleteReport(props.report.reportID)}
>

We are applying OfflineWithFeedback to both ReportActionItemParentAction and ReportActionItemCreated with the same props

<OfflineWithFeedback
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}
errorRowStyles={[styles.ml10, styles.mr2]}
onClose={() => Report.navigateToConciergeChatAndDeleteReport(props.report.reportID)}
needsOffscreenAlphaCompositing
>

So that the parent message of the thread will be wrapped by 2 OfflineWithFeedback Components. It makes the parent message of the thread become darker gray than other comments

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

We should remove the OfflineWithFeedback in ReportActionItemParentAction

Result

image

@ginsuma
Copy link
Contributor

ginsuma commented Aug 8, 2023

Proposal

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

Inconsistency: Different grayness levels on parent and child message

What is the root cause of that problem?

The ReportActionItemParentAction content is wrapped by two OfflineWithFeedback here and here.
When adding a message and creating a thread with it in offline mode, both will have opacity. This makes that item looks different than only one OfflineWithFeedback wrapper.

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

Solution 1:
We add a new prop called isDisabledOpacity to OfflineWithFeedback, and change needsOpacity here to:

const needsOpacity = !props.isDisabledOpacity && (...)

In ReportActionItemParentAction, we use that prop to avoid setting opacity when ReportActionItem will set it.

<OfflineWithFeedback
    isDisabledOpacity={parentReportAction.pendingAction}

Solution 2:
No need to add a new prop, but it will show only one error message.

return parentReportAction.pendingAction ? (
   {reportActionContent}
) : (
    <OfflineWithFeedback
        ...
    >
        {reportActionContent}
    </OfflineWithFeedback>
);

@joekaufmanexpensify
Copy link
Contributor

I can reproduce this. I can't think of any reason why we'd want parent and child messages to be in a different shade of grey here. Especially as the parent and child messages are normally the same text color while online.

2023-08-08_16-31-23.mp4

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 8, 2023
@melvin-bot melvin-bot bot changed the title Inconsistency: Different grayness levels on parent and child message [$1000] Inconsistency: Different grayness levels on parent and child message Aug 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0192daee63c42d4dbd

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

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

@ares-dev05
Copy link

Proposal

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

Different grayness levels

What is the root cause of that problem?

image
When you are offline status, the parent characters have opacity 0.5 but the child characters have opacity 1.

This is why needsOpacity in OfflineWithFeedback.js is false in the child message component.

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

So we have to enable needsOpacity to true in the child OfflineWithFeedback.js as well. For it, we can update the line 99 in OfflineWithFeedback.js as the following.

const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError || props.network.isOffline;

What alternative solutions did you explore? (Optional)

@joekaufmanexpensify
Copy link
Contributor

Pending review of proposals

@henryhover
Copy link

henryhover commented Aug 9, 2023

Proposal

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

The level of grayness on the Parent and child messages are different while offline

What is the root cause of that problem?

<OfflineWithFeedback
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}
errorRowStyles={[styles.ml10, styles.mr2]}
onClose={() => Report.navigateToConciergeChatAndDeleteReport(props.report.reportID)}
>
<View style={StyleUtils.getReportWelcomeContainerStyle(props.isSmallScreenWidth)}>
<View style={[styles.p5, StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]} />
{parentReportAction && (
<ReportActionItem
report={props.report}
action={parentReportAction}
displayAsGroup={false}
isMostRecentIOUReportAction={false}
shouldDisplayNewMarker={props.shouldDisplayNewMarker}
index={0}
/>
)}
</View>
{!props.shouldHideThreadDividerLine && <View style={[styles.threadDividerLine]} />}
</OfflineWithFeedback>

ReportActionItemParentAction component renders ReportActionItem component wrapped by OfflineWithFeedback component

<OfflineWithFeedback
onClose={() => ReportActions.clearReportActionErrors(props.report.reportID, props.action)}
pendingAction={props.draftMessage ? null : props.action.pendingAction}
shouldHideOnDelete={!ReportActionsUtils.hasCommentThread(props.action)}
errors={props.action.errors}
errorRowStyles={[styles.ml10, styles.mr2]}
needsOffscreenAlphaCompositing={ReportActionsUtils.isMoneyRequestAction(props.action)}
>
{isWhisper && (
<View style={[styles.flexRow, styles.pl5, styles.pt2]}>
<View style={[styles.pl6, styles.mr3]}>
<Icon
src={Expensicons.Eye}
small
/>
</View>
<Text style={[styles.chatItemMessageHeaderTimestamp]}>
{props.translate('reportActionContextMenu.onlyVisible')}
&nbsp;
</Text>
<DisplayNames
fullTitle={ReportUtils.getWhisperDisplayNames(whisperedToAccountIDs)}
displayNamesWithTooltips={displayNamesWithTooltips}
tooltipEnabled
numberOfLines={1}
textStyles={[styles.chatItemMessageHeaderTimestamp]}
shouldUseFullTitle={isWhisperOnlyVisibleByUser}
/>
</View>
)}
{renderReportActionItem(hovered, isWhisper)}
</OfflineWithFeedback>

And ReportActionItem also renders content wrapped by OfflineWithFeedback component

So the parent report content is wrapped by 2 OfflineWithFeedback Components, thereby we have darker grey

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

We should make ReportActionItemParentAction has only one OfflineWithFeedback component
Removing OfflineWithFeedback component from ReportActionItemParentAction can fix the issue visually but it is not good solution I think

In my opinion, it would be better to add new property(isParentPendingAction) to ReportActionItem component, and update pendingAction property below

pendingAction={props.draftMessage ? null : props.action.pendingAction}

to

pendingAction={props.draftMessage ? null : props.action.pendingAction && !props.isParentPendingAction}

So the nested OfflineWithFeedback component won't have opacity

My details
Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01bfdbf8afd2b5348a

@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2023

📣 @henryhover! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@joekaufmanexpensify
Copy link
Contributor

@sobitneupane thoughts on these proposals?

@joekaufmanexpensify
Copy link
Contributor

Bumped in Slack.

@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2023
@sobitneupane
Copy link
Contributor

Thanks for the proposal @tienifr.

I don't think I agree with your RCA. Is ReportActionItemCreated is being used in thread? I think it is simply being used to show report welcome message and we are not showing one in thread.

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2023
@sobitneupane
Copy link
Contributor

sobitneupane commented Aug 14, 2023

Thanks for the proposal @ginsuma

The ReportActionItemParentAction content is wrapped by two OfflineWithFeedback

If ReportActionItemParentAction is wrapped by two OfflineWithFeedback, rather than introducing a new prop can we simply remove OfflineWithFeedback from ReportActionItemParentAction as suggested by @tienifr in his proposal?

@ginsuma
Copy link
Contributor

ginsuma commented Aug 14, 2023

Proposal

Update
I think we should keep both of them because they have different pending actions and errors. In ReportActionItemParentAction, it is related to the thread. In ReportActionItem, it is the report action itself.
We will need to show two error messages, right?
If not, we can go with 2nd solution in my proposal.

<OfflineWithFeedback
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}

cc: @sobitneupane

@kaushiktd
Copy link
Contributor

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

Inconsistency: Different grayness levels on parent and child message

What is the root cause of that problem?

when we are offline the Parent character opacity is 0.5 and child character opacity is 1, opacity value is different for parent and child components.

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

To solve this issue you need to create a new class in style.js on below mention file
https://github.com/Expensify/App/blob/main/src/styles/styles.js#L2939

offlineOpacity: {
opacity:1,
},

and also change class in below file:
https://github.com/Expensify/App/blob/main/src/components/OfflineWithFeedback.js#L112

<View style={props.style}>
{!hideChildren && (
<View
style={[needsOpacity ? styles.offlineFeedback.offlineOpacity : {}, props.contentContainerStyle]}
needsOffscreenAlphaCompositing={shouldRenderOffscreen ? needsOpacity && props.needsOffscreenAlphaCompositing : undefined}
>
{children}
</View>
)}

Video
https://drive.google.com/file/d/1OqBop-wxBDxkMvXeqlcDx7UCZlOQzfup/view?usp=drive_link

@melvin-bot
Copy link

melvin-bot bot commented Aug 15, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@joekaufmanexpensify
Copy link
Contributor

@sobitneupane do you think we're close to being able to use any of these proposals?

@DylanDylann
Copy link
Contributor

DylanDylann commented Aug 16, 2023

Proposal

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

Inconsistency: Different grayness levels on parent and child message

What is the root cause of that problem?

We are applying 2 OfflineWithFeedback to the first message of child thread

<OfflineWithFeedback
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}
errorRowStyles={[styles.ml10, styles.mr2]}
onClose={() => Report.navigateToConciergeChatAndDeleteReport(props.report.reportID)}
>

<OfflineWithFeedback
onClose={() => ReportActions.clearReportActionErrors(props.report.reportID, props.action)}
pendingAction={props.draftMessage ? null : props.action.pendingAction}
shouldHideOnDelete={!ReportActionsUtils.hasCommentThread(props.action)}
errors={props.action.errors}
errorRowStyles={[styles.ml10, styles.mr2]}
needsOffscreenAlphaCompositing={ReportActionsUtils.isMoneyRequestAction(props.action)}

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

We should remove OfflineWithFeedback in ReportActionItemParentAction. And then, in ReportActionItemParentAction we will pass new prop called offlineObject like this:

offlineObject: {
            pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
            errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}
            onClose={() => Report.navigateToConciergeChatAndDeleteReport(props.report.reportID)}
}

And in ReportActionItem, if offlineObject props exist we will use it as params of OfflineWithFeedback Component else we still use the current condition

What alternative solutions did you explore? (Optional)

@techievivek
Copy link
Contributor

@joekaufmanexpensify Merge freeze is over now https://expensify.slack.com/archives/C01GTK53T8Q/p1694405959277169 so I went ahead and merged the PR.

@techievivek techievivek changed the title [HOLD for merge freeze] [$1000] Inconsistency: Different grayness levels on parent and child message [$1000] Inconsistency: Different grayness levels on parent and child message Sep 11, 2023
@techievivek techievivek changed the title [$1000] Inconsistency: Different grayness levels on parent and child message [HOLD for payment 2023-09-18][$1000] Inconsistency: Different grayness levels on parent and child message Sep 11, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 13, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-09-18][$1000] Inconsistency: Different grayness levels on parent and child message [HOLD for payment 2023-09-20] [HOLD for payment 2023-09-18][$1000] Inconsistency: Different grayness levels on parent and child message Sep 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

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

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

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:

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
Copy link

melvin-bot bot commented Sep 13, 2023

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:

@joekaufmanexpensify
Copy link
Contributor

@sobitneupane mind completing the BZ checklist here so we can prep to issue payment?

@joekaufmanexpensify
Copy link
Contributor

Bump @sobitneupane mind working on the BZ checklist so we can finish this one off?

@joekaufmanexpensify
Copy link
Contributor

Bumped in Slack

@sobitneupane
Copy link
Contributor

sobitneupane commented Sep 25, 2023

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:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#18522

  • [@sobitneupane] 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:

#18522 (comment)

  • [@sobitneupane] 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:

Probably offline tests were not performed in the linked PR. This should have been caught in offline test.

  • [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

  • [@sobitneupane] 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.

#24263 (comment)

@sobitneupane
Copy link
Contributor

Regression Test Proposal

  • Be Offline
  • Go to any chat
  • Send a message
  • Reply in thread
  • Send a reply and verify that there is no difference in the grayness between parent and child messages.

@sobitneupane
Copy link
Contributor

I will request payment in newDot once the payment is summarized.

cc: @joekaufmanexpensify

@joekaufmanexpensify
Copy link
Contributor

Great, thanks! I will work on the BZ checklist.

@joekaufmanexpensify joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 labels Sep 26, 2023
@joekaufmanexpensify
Copy link
Contributor

IMO, this is a very minor visual bug, and no regression test is needed.

@joekaufmanexpensify
Copy link
Contributor

BZ checklist is complete, all set to issue payment. We need to make the following payments:

  • @daveSeife - $250 for report. Paid via Upwork.
  • @ginsuma - $500 for PR. (Originally $1,000 and reduced 50% because of regression. Regression also negates urgency bonus.) Paid via Upwork.
  • @sobitneupane - $500 for C+. (Originally $1,000 and reduced 50% because of regression. Regression also negates urgency bonus.) Paid via NewDot.

@joekaufmanexpensify
Copy link
Contributor

@daveSeife $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@ginsuma $500 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@sobitneupane could you please request $500 and confirm here once that's done? TY!

@daveSeife
Copy link

@joekaufmanexpensify received. Thank you!

@joekaufmanexpensify
Copy link
Contributor

Pending request

@joekaufmanexpensify
Copy link
Contributor

Closing per our slack discussion! Payment summary message for when @sobitneupane requests is here.

@sobitneupane
Copy link
Contributor

#24263 (comment)

Requested payment on newDot

@JmillsExpensify
Copy link

$500 payment approved for @sobitneupane based on BZ summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests