[HOLD for payment 2024-10-24] [HOLD for payment 2024-10-16] Add LHN debugging to Debug Mode

puneetlath commented 2 months ago

We are adding a debug mode to the App here: https://github.com/Expensify/App/pull/45481

What I would really love from the Debug Mode is something that tells me why a given report is in the LHN. So if I'm seeing a report in the LHN, why is it there? And similarly if it has an RBR/GBR, what is causing that to be there? This might be more useful in Focus Mode than in Most Recent, but I think it would help with a lot of the recent issues we've been troubleshooting.

cc @pac-guerreiro

Issue Owner

Current Issue Owner: @puneetlath

melvin-bot[bot] commented 2 months ago

Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

pac-guerreiro commented 2 months ago

Hi! I’m Pedro Guerreiro from Callstack - expert contributor group. I’d like to work on this task!

puneetlath commented 2 months ago

Still waiting for the first PR to get merged.

DylanDylann commented 2 months ago

yeah, I will try to complete check list today

puneetlath commented 2 months ago

Still waiting for the first PR to get merged.

puneetlath commented 2 months ago

Still waiting for the first PR to get merged.

puneetlath commented 1 month ago

Still waiting for the first PR to get merged.

blimpich commented 1 month ago

@puneetlath can you respond to this comment on the PR?

fabioh8010 commented 1 month ago

Update: PR is being reviewed by C+, should be completed before next week.

blimpich commented 1 month ago

Assigning @DylanDylann since they are reviewing the PR

blimpich commented 1 month ago

I'll be OOO starting this afternoon until October 14th. @puneetlath can you give the final approval once the PR is ready?

neil-marcellini commented 1 month ago

Taking over as Ben asked and because Puneet is out as well

puneetlath commented 4 weeks ago

Thanks @neil-marcellini!

Something I realized with this, will we be able to see why a report is in the LHN without clicking into it? For example, I want to know why these reports are in my LHN, but I don't want to click into them and have OpenReport be called.

DylanDylann commented 4 weeks ago

Agreed. I see that we can't know why these reports display in LHN now because if we click on the report and check the debug tab on the report detail page, It means that we are focusing on this report and the reason will always be "Is temporarily focused"

puneetlath commented 4 weeks ago

@pac-guerreiro what would you think of being able to open the debug pane for a given report via right click, without clicking into it.

pac-guerreiro commented 4 weeks ago

@puneetlath I will give it a try to see if it won't trigger OpenReport too

pac-guerreiro commented 3 weeks ago

@puneetlath

Sorry for the delay, I got caught with other tasks related to debug mode 😅

Here is a preview of the change you requested: https://github.com/user-attachments/assets/8f47255b-9d89-46d7-9270-dbbcb364a6b8

It's safe to say that opening debug page from context menu won't trigger the OpenReport API call

puneetlath commented 3 weeks ago

Looks great to me!

pac-guerreiro commented 3 weeks ago

@puneetlath

Awesome 😄 Should there be a new issue for this or do you want me to just add this in a new PR?

puneetlath commented 3 weeks ago

Let's just do another PR attached to this issue. @DylanDylann can review again since they have context.

melvin-bot[bot] commented 3 weeks ago

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

https://github.com/Expensify/App/pull/48442

If no regressions arise, payment will be issued on 2024-10-16. :confetti_ball:

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

@pac-guerreiro does not require payment (Contractor)
@DylanDylann requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 3 weeks ago

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

[ ] [@DylanDylann] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
[ ] [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

pac-guerreiro commented 3 weeks ago

@puneetlath @DylanDylann

Here is the PR that adds the debug option to LHN context menu - https://github.com/Expensify/App/pull/50519 😄

DylanDylann commented 3 weeks ago

@pac-guerreiro Currently, we don't display the reason why RBR is displayed. But let's see logic to display RBR on LHN

https://github.com/Expensify/App/blob/12e41b177cc0a3dc1c4b26beceb28d337e69a37d/src/libs/SidebarUtils.ts#L237-L255

We have three cases where the RBR is displayed on LHN

shouldDisplayTransactionThreadViolations return true
hasErrors (errors come from report or reportActions. Usually, this will be returned from BE)
hasViolations

We need to design three reason messages for each case

puneetlath commented 3 weeks ago

Yes, let's add that. I think it would be very helpful for diagnosing why RBRs are showing.

pac-guerreiro commented 3 weeks ago

@puneetlath can you create a new issue for me to implement this? 😄

puneetlath commented 3 weeks ago

Yep. Here you go: https://github.com/Expensify/App/issues/50665

pac-guerreiro commented 3 weeks ago

Thanks @puneetlath, that was fast 😄

melvin-bot[bot] commented 2 weeks ago

Payment Summary

[Upwork Job]()

Contributor: @pac-guerreiro is from an agency-contributor and not due payment
ROLE: @DylanDylann paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@puneetlath)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

puneetlath commented 2 weeks ago

@DylanDylann are we all done here?

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

https://github.com/Expensify/App/pull/50519

If no regressions arise, payment will be issued on 2024-10-24. :confetti_ball:

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

@pac-guerreiro does not require payment (Contractor)
@DylanDylann requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 2 weeks ago

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

[ ] [@DylanDylann] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
[ ] [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

DylanDylann commented 2 weeks ago

@puneetlath Do we require the regression test on debug feature issues?

puneetlath commented 2 weeks ago

Yes please. We rely on these features quite a bit, so it'd be good to have some regression testing to make sure we don't break them accidentally. Otherwise we won't realize they are broken until we need them to help us debug something.

DylanDylann commented 2 weeks ago

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: NA [@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: NA [@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: NA [@DylanDylann] Determine if we should create a regression test for this bug. Yes [@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.

Regression Test Proposal 1

Enable the Debug feature
On the home page, open a chat report from LHN
Click on the header of the chat you just opened, then click on Debug
On the Details tab, confirm that "Visible in LHN" provides the correct reason for the report to be shown on LHN
Confirm that "GBR" shows the correct reason, if the report has GBR in LHN
Confirm that "RBR" shows the correct reason, if the report has RBR in LHN

Regression Test Proposal 2

Enable the Debug feature
On the home page, right-click or long-press a report from LHN,
Select Debug option
The debug page is displayed

Do we agree 👍 or 👎

melvin-bot[bot] commented 1 week ago

Payment Summary

[Upwork Job]()

Contributor: @pac-guerreiro is from an agency-contributor and not due payment
ROLE: @DylanDylann paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@puneetlath)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

puneetlath commented 1 week ago

@DylanDylann were there multiple PRs for this one?

DylanDylann commented 1 week ago

@puneetlath Initially we implemented this first PR, after that we have a new requirement to add debug option to context menu on LHN, the second PR was created to address this. Finally, we created the third PR to add RBR reason to debug mode that we missed in the first PR (Note that: we already have an issue for this problem)

DylanDylann commented 1 week ago

I also updated the regression test to cover all implementations

Expensify / App