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 2024-05-22] [$250] mWeb - In room,inviting an user& creating split scan not displaying Scan in progress in lhn #40557

Closed
1 of 6 tasks
kavimuru opened this issue Apr 19, 2024 · 22 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 Apr 19, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.63-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: EXP around https://expensify.testrail.io/index.php?/tests/view/4495609
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

  1. Go to https://staging.new.expensify.com/

  2. Tap fab -- start chat -- room

  3. Create a room
    4.Tap header---members and invite a member
    5.Navigate back to room page
    6.Tap plus--- split expense-- scan
    7.Upload an image using camera
    8.Tap split expense
    9.Note scan in progress shown in preview
    10.Open the expense and note header message shown scanning
    11.Navigate to LHN

Expected Result:

In room, after inviting an user& creating split scan expense must display Scan in progress message in lhn.

Actual Result:

In room, after inviting an user& creating split scan expense displays invited 1user message in LHN instead of Scan in progress message.

Workaround:

unknown

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

Bug6454325_1713479892547.rov.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0169b520297273f864
  • Upwork Job ID: 1782362477239189504
  • Last Price Increase: 2024-04-22
  • Automatic offers:
    • bernhardoj | Contributor | 0
Issue OwnerCurrent Issue Owner: @zanyrenney
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 19, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

Triggered auto assignment to @zanyrenney (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.

@kavimuru
Copy link
Author

@zanyrenney FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@kavimuru
Copy link
Author

This bug might be related to #vip-vsb

@bernhardoj
Copy link
Contributor

Proposal

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

When doing a split bill scan in a room, the LHN last message doesn't show Scan in progress. If we do this in, let say a group chat, the LHN last message will show Scan in progress.

What is the root cause of that problem?

If it's a chat room, the last message value will be based on the report lastAction.

const lastAction = visibleReportActionItems[report.reportID];
const isThreadMessage =
ReportUtils.isThread(report) && lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT && lastAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
if ((result.isChatRoom || result.isPolicyExpenseChat || result.isThread || result.isTaskReport || isThreadMessage) && !result.isArchivedRoom) {
if (lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED) {
const newName = lastAction?.originalMessage?.newName ?? '';
result.alternateText = Localize.translate(preferredLocale, 'newRoomPage.roomRenamedTo', {newName});
} else if (ReportActionsUtils.isTaskAction(lastAction)) {
result.alternateText = ReportUtils.formatReportLastMessageText(TaskUtils.getTaskReportActionMessage(lastAction).text);
} else if (
lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOM_CHANGE_LOG.INVITE_TO_ROOM ||
lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOM_CHANGE_LOG.REMOVE_FROM_ROOM ||
lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.INVITE_TO_ROOM ||
lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.REMOVE_FROM_ROOM
) {
const targetAccountIDs = lastAction?.originalMessage?.targetAccountIDs ?? [];
const verb =
lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.ROOM_CHANGE_LOG.INVITE_TO_ROOM || lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.INVITE_TO_ROOM
? Localize.translate(preferredLocale, 'workspace.invite.invited')
: Localize.translate(preferredLocale, 'workspace.invite.removed');
const users = Localize.translate(preferredLocale, targetAccountIDs.length > 1 ? 'workspace.invite.users' : 'workspace.invite.user');
result.alternateText = `${lastActorDisplayName} ${verb} ${targetAccountIDs.length} ${users}`.trim();
const roomName = lastAction?.originalMessage?.roomName ?? '';
if (roomName) {
const preposition =
lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.ROOM_CHANGE_LOG.INVITE_TO_ROOM || lastAction.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.INVITE_TO_ROOM
? ` ${Localize.translate(preferredLocale, 'workspace.invite.to')}`
: ` ${Localize.translate(preferredLocale, 'workspace.invite.from')}`;
result.alternateText += `${preposition} ${roomName}`;
}
} else if (lastAction?.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.LEAVE_POLICY) {
result.alternateText = Localize.translateLocal('workspace.invite.leftWorkspace');
} else if (lastAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW && lastActorDisplayName && lastMessageTextFromReport) {
result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;
} else {
result.alternateText = lastMessageTextFromReport.length > 0 ? lastMessageText : ReportActionsUtils.getLastVisibleMessage(report.reportID, {}, lastAction)?.lastMessageText;
if (!result.alternateText) {
result.alternateText = Localize.translate(preferredLocale, 'report.noActivityYet');
}
}

The last action is taken from visibleReportActionItems object. To get visibleReportActionItems, we sort the report actions, filter it, then take the last item.

const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(actions));
// The report is only visible if it is the last action not deleted that
// does not match a closed or created state.
const reportActionsForDisplay = actionsArray.filter(
(reportAction, actionKey) =>
ReportActionsUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
!ReportActionsUtils.isWhisperAction(reportAction) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
);
visibleReportActionItems[reportID] = reportActionsForDisplay[reportActionsForDisplay.length - 1];

Notice that in the filter, we filter out any whisper action, which is true for scan requests while scanning. So, the scan request is never taken into consideration and the last message will be the previous message.

But as I mentioned in the first section above, this doesn't happen when we do the split bill scan in a group chat for example. That's because, for group chat, we get the last message from lastMessageText,

result.alternateText =
(ReportUtils.isGroupChat(report) || ReportUtils.isDeprecatedGroupDM(report)) && lastActorDisplayName
? `${lastActorDisplayName}: ${lastMessageText}`
: lastMessageText || formattedLogin;

and lastMessageText comes from OptionsListUtils.getLastMessageTextForReport.

const lastMessageTextFromReport = OptionsListUtils.getLastMessageTextForReport(report, lastActorDetails, policy);
// We need to remove sms domain in case the last message text has a phone number mention with sms domain.
let lastMessageText = Str.removeSMSDomain(lastMessageTextFromReport);

In OptionsListUtils.getLastMessageTextForReport, the way we get the last report action is by filtering it only with a ReportActionUtils.shouldReportActionBeVisibleAsLastAction function.

function getLastMessageTextForReport(report: OnyxEntry<Report>, lastActorDetails: Partial<PersonalDetails> | null, policy?: OnyxEntry<Policy>): string {
const lastReportAction = allSortedReportActions[report?.reportID ?? '']?.find((reportAction) => ReportActionUtils.shouldReportActionBeVisibleAsLastAction(reportAction)) ?? null;

In ReportActionUtils.shouldReportActionBeVisibleAsLastAction, we will only filter out whisper messages if it's not a report preview or IOU action.

!(isWhisperAction(reportAction) && !isReportPreviewAction(reportAction) && !isMoneyRequestAction(reportAction)) &&

That's why the split bill scan request is taken into consideration as the last action.

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

We should make the last action logic consistent for the last message text. In SidebarUtils itself, we have 3 ways to get the last action. One is the last action that doesn't get through the filter, two get through the filter (one filter with ReportActionUtils.shouldReportActionBeVisibleAsLastAction and the other one is the same as in SidebarUtils)

// some types of actions are filtered out for lastReportAction, in some cases we need to check the actual last action
const lastOriginalReportAction = lastReportActions[report?.reportID ?? ''] ?? null;

const lastReportAction = allSortedReportActions[report?.reportID ?? '']?.find((reportAction) => ReportActionUtils.shouldReportActionBeVisibleAsLastAction(reportAction)) ?? null;

const reportActionsForDisplay = sortedReportActions.filter(
(reportAction, actionKey) =>
ReportActionUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
!ReportActionUtils.isWhisperAction(reportAction) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
);
visibleReportActionItems[reportID] = reportActionsForDisplay[reportActionsForDisplay.length - 1];

const lastAction = visibleReportActionItems[report.reportID];

Here is the plan:

  1. In SidebarUtils and OptionsListUtils, update the last action filter to use ReportActionsUtils.shouldReportActionBeVisibleAsLastAction(reportAction).
const reportActionsForDisplay = actionsArray.filter((reportAction) => ReportActionsUtils.shouldReportActionBeVisibleAsLastAction(reportAction));

// SidebarUtils
visibleReportActionItems[reportID] = reportActionsForDisplay[reportActionsForDisplay.length - 1];

// OptionsListUtils - the report actions are sorted in descending order
visibleReportActionItems[reportID] = reportActionsForDisplay[0];
  1. Replace the filtering code below to const lastReportAction = visibleReportActionItems[report?.reportID ?? ''] ?? null;
    function getLastMessageTextForReport(report: OnyxEntry<Report>, lastActorDetails: Partial<PersonalDetails> | null, policy?: OnyxEntry<Policy>): string {
    const lastReportAction = allSortedReportActions[report?.reportID ?? '']?.find((reportAction) => ReportActionUtils.shouldReportActionBeVisibleAsLastAction(reportAction)) ?? null;

NOTE:
a. We can move the logic to store the last action to ReportActionsUtils and create a new util function called getLastActionForLastMessageText.
b. I removed the filtering condition for CREATED action because I think it's not needed. We can add it back if we want to play safe.
c. If we don't want to include a report preview while scanning, then we just need to perform step 2 above (and access the first item instead of the last as shown in step 1).

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 22, 2024
@zanyrenney
Copy link
Contributor

Agree, this looks related to VIP VSB

@zanyrenney zanyrenney added the External Added to denote the issue can be worked on by a contributor label Apr 22, 2024
Copy link

melvin-bot bot commented Apr 22, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0169b520297273f864

@melvin-bot melvin-bot bot changed the title mWeb - In room,inviting an user& creating split scan not displaying Scan in progress in lhn [$250] mWeb - In room,inviting an user& creating split scan not displaying Scan in progress in lhn Apr 22, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 22, 2024
Copy link

melvin-bot bot commented Apr 22, 2024

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

@zanyrenney
Copy link
Contributor

Already have a proposal above! @thesahindia please take a look and let me know if we can hire @bernhardoj

@melvin-bot melvin-bot bot added the Overdue label Apr 24, 2024
@zanyrenney
Copy link
Contributor

bump @thesahindia if you don't have capacity to review the proposal, please let me know and I will assign another C+

@melvin-bot melvin-bot bot removed the Overdue label Apr 25, 2024
@thesahindia
Copy link
Member

@bernhardoj's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 26, 2024

Triggered auto assignment to @yuwenmemon, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 26, 2024
Copy link

melvin-bot bot commented Apr 26, 2024

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added the Weekly KSv2 label Apr 29, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @thesahindia

Copy link

melvin-bot bot commented May 7, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@bernhardoj
Copy link
Contributor

The PR is reverted because of a regression. The new PR is ready

cc: @thesahindia

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels May 8, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 15, 2024
@melvin-bot melvin-bot bot changed the title [$250] mWeb - In room,inviting an user& creating split scan not displaying Scan in progress in lhn [HOLD for payment 2024-05-22] [$250] mWeb - In room,inviting an user& creating split scan not displaying Scan in progress in lhn May 15, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 15, 2024
Copy link

melvin-bot bot commented May 15, 2024

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

Copy link

melvin-bot bot commented May 15, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.73-7 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 2024-05-22. 🎊

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

Copy link

melvin-bot bot commented May 15, 2024

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:

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 21, 2024
@zanyrenney
Copy link
Contributor

@thesahindia please complete the checklist above before I can issue payment. Thanks!

@thesahindia
Copy link
Member

thesahindia commented May 24, 2024

We were filtering out all whisper actions, which caused this.

Test case steps -

  1. Open a room chat
  2. Invite any user
  3. Verify the LHN of the room shows "invited x user"
  4. Create a new split scan request in the room
  5. Verify the LHN of the room shows "Scan in progress.."

@zanyrenney
Copy link
Contributor

payment summary

paid $250 to @bernhardoj via UW.
@thesahindia is paid via ND. They are owed $250.

Closing!

@JmillsExpensify
Copy link

$250 approved for @thesahindia

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
No open projects
Archived in project
Development

No branches or pull requests

6 participants