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

[$1000] Web - Number of replies are not dynamically updating when user adds comments by editing description #25854

Closed
1 of 6 tasks
izarutskaya opened this issue Aug 24, 2023 · 26 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 24, 2023

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


Action Performed:

  1. Make a money request from account A to account B
  2. Open the IOU details page by clicking on the IOU preview component twice on account A
  3. Go to account A and post something and observe the reply count on account B
  4. Change the amount first then description (observe the reply count on account B)
  5. Reload the page on account B and observe the reply count again

Expected Result:

Reply count should update dynamically without manual reloading

Actual Result:

Reply count does not update dynamically if account A updates the IOU description field

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: v1.3.57-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

2023-08-15.00.03.20.mp4
20230824_182534.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Nathan-Mulugeta

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692047239826689

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0138b43dd9b2590146
  • Upwork Job ID: 1697702739222093824
  • Last Price Increase: 2023-09-18
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 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

@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 29, 2023
@stephanieelliott
Copy link
Contributor

I agree that this is something we should fix, adding External label

@melvin-bot melvin-bot bot removed the Overdue label Sep 1, 2023
@stephanieelliott stephanieelliott added External Added to denote the issue can be worked on by a contributor Overdue labels Sep 1, 2023
@melvin-bot melvin-bot bot changed the title Web - Number of replies are not dynamically updating when user adds comments by editing description [$500] Web - Number of replies are not dynamically updating when user adds comments by editing description Sep 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 1, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0138b43dd9b2590146

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

melvin-bot bot commented Sep 1, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 1, 2023

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

@stephanieelliott
Copy link
Contributor

Labels added, not overdue

@melvin-bot melvin-bot bot removed the Overdue label Sep 1, 2023
@imranaalam
Copy link

imranaalam commented Sep 2, 2023

Issue: Reply Count Not Dynamically Updating on IOU Description Change

Problem Overview:

When a user updates the IOU description field, the reply count doesn't update in real-time, requiring a manual page reload for the update to be reflected.

Steps to Reproduce:

  1. Make a money request from account A to account B over the internet.
  2. Open the IOU details page by clicking on the IOU preview component twice on account A.
  3. Post something on account A and observe the reply count on account B — it increases.
  4. Change the amount, then the description on account A. Observe the reply count on account B — it doesn't change.
  5. Reload the page on account B. The reply count now reflects the change.

Expected Behavior:
The reply count should dynamically update without manual reloading, as observed in step 3.

Actual Behavior:
The reply count does not update dynamically when the IOU description field is modified in account A.

Workaround:
Currently unknown.

Initial Findings from Code Inspection:

  • Inspection of IOU.js revealed functions setMoneyRequestDescription and setMoneyRequestAmount both use Onyx.merge to update the Onyx store.
  • TransactionUtils.js contains the getDescription function, which fetches the IOU description. An accompanying comment indicates the absence of a modifiedComment counterpart for the description.

Key Insight:

Many systems maintain both original and "modified" versions of a property for change tracking. The lack of a modifiedComment counterpart for the IOU description suggests that updates to the description might not trigger real-time actions in the same way as other properties.

Suggested Next Steps for Investigation:

  • Inquire into the Absence of modifiedComment: Understand why the IOU description lacks a modifiedComment counterpart and how this omission might impact real-time update mechanisms.

  • Review Backend/API Communication: Ensure that when the IOU description updates, the backend/API sends real-time updates to all connected clients. This may necessitate examining websockets or similar real-time communication protocols.

  • Check Components Displaying IOU Description: Review components interacting with the IOU description to ensure they're subscribed correctly to the relevant Onyx key and set up for dynamic updates.

  • Examine Tracking of Transaction Property Changes: Dive into how changes to transaction properties (with both original and modified counterparts) are monitored and the mechanisms triggering real-time updates for these properties.

@parasharrajat
Copy link
Member

@imranaalam
Copy link

imranaalam commented Sep 3, 2023

Problem Statement:
The system does not reflect real-time updates when changes are made to the IOU description, also known as the comment.

Root Cause:

  1. Comment Structure: The comment is not just a simple string; it's an object with potentially multiple properties. This includes the primary comment, source, and originalTransactionID. The fact that it's a composite object makes introducing a direct modifiedComment counterpart challenging due to the complexity in managing such structures.

  2. IOU Description in Comment: While the comment is referred to as the IOU description in the app, it can store other data related to the transaction, not just the description. The presence of additional fields like source and originalTransactionID in the commentJSON structure underscores its multifunctionality.

  3. SmartScan Interactions: The code contains logic to interact with SmartScan, a feature likely used to process and extract information from receipts. The changes made to properties such as merchant, amount, and currency influence the behavior of the SmartScan. Introducing a modifiedComment could interact or conflict with this SmartScan process.

  4. pendingFields Mechanism: The system uses the pendingFields property to track pending changes to a transaction. If modifiedComment were introduced, it would necessitate a corresponding mechanism in pendingFields to handle its state.

Given the multifunctionality of the comment property and its interactions with features like SmartScan, introducing a straightforward modifiedComment counterpart is challenging.

Proposed Solution:

  1. Introduce a Change Tracker for Comment:
    Rather than adding a modifiedComment counterpart, use a commentChange property. This boolean flag indicates if the main comment description has been updated.

    if (_.has(transactionChanges, 'comment')) {
        updatedTransaction.comment = {
            ...updatedTransaction.comment,
            comment: transactionChanges.comment,
        };
        updatedTransaction.commentChange = true;
    }

    This approach offers a lightweight change detection mechanism.

  2. Handle SmartScan Interactions:
    Ensure that any change tracking mechanism for the comment does not interfere with SmartScan. Test to confirm that changes to the comment don't trigger unintended behaviors in SmartScan or related systems.

  3. Update pendingFields Mechanism:
    Reflect changes to the comment in the pendingFields mechanism.

    updatedTransaction.pendingFields = {
        ...updatedTransaction.pendingFields,
        ...(_.has(transactionChanges, 'comment') && {comment: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}),
    };
  4. Enhance Real-time Update Mechanism:
    Ensure components displaying the IOU description (comment) are subscribed to the relevant Onyx key for dynamic updates. Confirm that the backend or API sends real-time updates when the IOU description changes.

  5. Rigorous Testing:
    Test the solutions, especially in areas where the comment interacts with other features like SmartScan, to prevent unexpected side effects or regressions.

Alternative Solution:
Consider enhancing the real-time update mechanism in the system holistically. Ensure all transaction properties, not just the comment, are updated in real-time:

  • Make sure all clients are subscribed to the necessary data changes.
  • Ensure the backend or middleware pushes changes to clients instantly.
  • Implement a robust change detection mechanism that captures all modifications to the transaction.

This approach might provide a more consistent real-time update behavior across all properties.

@parasharrajat
Copy link
Member

Waiting for more proposals.

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Sep 8, 2023
@stephanieelliott
Copy link
Contributor

This has been open a while without many proposals, doubling the bounty on this one.

@melvin-bot melvin-bot bot removed the Overdue label Sep 11, 2023
@stephanieelliott stephanieelliott changed the title [$500] Web - Number of replies are not dynamically updating when user adds comments by editing description [$1000] Web - Number of replies are not dynamically updating when user adds comments by editing description Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

Upwork job price has been updated to $1000

@pradeepmdk
Copy link
Contributor

could you double-check the requirements? because now any changing action is not countable (replay count)

you can check the latest version as per the bug video now Even if you changed the amount it does not count as a replay count.

@parasharrajat
Copy link
Member

@izarutskaya @stephanieelliott is this still a valid bug?

@melvin-bot
Copy link

melvin-bot bot commented Sep 14, 2023

@parasharrajat @stephanieelliott this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@akamefi202
Copy link
Contributor

I think this is BE issue. The FE only shows childVisibleActionCount value which is from BE.

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@parasharrajat, @stephanieelliott Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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

@rojiphil
Copy link
Contributor

@izarutskaya @stephanieelliott is this still a valid bug?

I too have the same question. Is this still a valid bug?

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2023

@parasharrajat, @stephanieelliott Still overdue 6 days?! Let's take care of this!

@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

@parasharrajat @stephanieelliott this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

Current assignee @parasharrajat is eligible for the Internal assigner, not assigning anyone new.

@stephanieelliott
Copy link
Contributor

I don't think this is happening anymore, I just retested and the reply count is now updating without having to reload the page 🎉

Tested on Chrome and Safari

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

8 participants