Revert #52569

[JmillsExpensify]

After giving the solution in #52569 a try, we've realized that it creates a confusing situation in the Inbox, which is:

• Workspace chat is GBRed
• Report has a grey Approved button

So in short, our solution for approve and pay (specifically with a report that has some held expenses and some unheld expenses) ends up breaking GBR.

As a result, we're going to revert the PR from #52569. That restores GBR to the Inbox and workspace chat. In addition, for the Search page specifically, we are going to disable the Approve button for this case, just like we landed on doing for Pay.

Issue Owner

Current Issue Owner: @mananjadhav

[melvin-bot]

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

[mananjadhav]

@JmillsExpensify The expectation is that we revert the PR, and then disable Approve button as well as Pay as a new requirement? Would it be a single PR or two PRs - one revert and then second to disable Approve?

I can help with the review on this one if needed.

[melvin-bot]

@JmillsExpensify, @luacmartins Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[luacmartins]

I think we want to:

1. Revert the behavior of changing the button colors.
2. On Search, disable the option in the bulk action dropdown if any selected items are on hold

[luacmartins]

@mananjadhav you're welcome to work on the PRs too if you'd like

[mananjadhav]

Yes, I can pick this.

[luacmartins]

Great! All yours ❤️

[luacmartins]

@mananjadhav how's this one going? Do you have an ETA for the PR?

[mananjadhav]

I am a bit sick, I'll have the Pr by tomorrow.

[luacmartins]

Ah no worries. Hope you feel better soon.

[melvin-bot]

@JmillsExpensify, @mananjadhav, @luacmartins Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mananjadhav]

Apologies I was unwell. I am just getting back and will have this ready in 1-2 days.

[melvin-bot]

@JmillsExpensify @mananjadhav @luacmartins this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@JmillsExpensify, @mananjadhav, @luacmartins Huh... This is 4 days overdue. Who can take care of this?

[mananjadhav]

Will raise the PR today.

[mananjadhav]

I think we want to:

1. Revert the behavior of changing the button colors.

I tried doing a straight revert and it didn't work. What I've done is reverted the changes of the props. We also added a prop for success style, do we want to revert that as well @luacmartins?

2. On Search, disable the option in the bulk action dropdown if any selected items are on hold

The current behavior is that the option is hidden. Are yo saying we want to add the option and keep it disabled?

web-bulk-option-approval.mov

[parasharrajat]

@mananjadhav Do you need help reviewing the PR?

[luacmartins]

I tried doing a straight revert and it didn't work. What I've done is reverted the changes of the props. We also added a prop for success style, do we want to revert that as well @luacmartins?

Yes, let's remove the prop as well. We currently only seem to use it if there are held expenses, which is unnecessary now that we want to change that behavior to always show it in success style.

The current behavior is that the option is hidden. Are yo saying we want to add the option and keep it disabled?

Yes, we should add the option to the dropdown and disable it if the user can't take the action

[luacmartins]

@parasharrajat assigned you as C+. Thanks!

[mananjadhav]

@luacmartins @parasharrajat Two questions:

1. Are you aware why do we disable the whole dropdown button if the selectedItem is disabled.

App/src/components/ButtonWithDropdownMenu/index.tsx

Line 133 in 04cd7b0

isDisabled={isDisabled || !!selectedItem?.disabled}

I think we should remove the !!selectedItem?.disabled especially because options.length > 1.

web-dropdown-button-disabled.mov

2. Should replicate the behavior for Pay button?

[luacmartins]

Are you aware why do we disable the whole dropdown button if the selectedItem is disabled.

I'm not sure. I think it'd make sense to disable the dropdown only if all options are disabled, not only the selected one.

Should replicate the behavior for Pay button?

Yes, I think we should do the same for all available bulk actions, i.e. Delete, Hold, Unhold, Approve, Pay

[mananjadhav]

Okay thanks for the clarification.

[mananjadhav]

@luacmartins I don't think isAnyTransactionOnHold would apply to Delete, Hold and Unhold right?

Or are you suggesting we update the condition respective to the buttons?

For example,

const shouldShowHoldOption = !isOffline && selectedTransactionsKeys.every((id) => selectedTransactions[id].canHold);

we remove selectedTransactionsKeys.every((id) => selectedTransactions[id].canHold); and always show Hold and disabled when selectedTransactionsKeys.every((id) => !selectedTransactions[id].canHold);

[luacmartins]

Ah no, only Approve and Pay are affected by the holds. Ignore my previous comment

[mananjadhav]

@parasharrajat The PR in draft is ready for review. I still need to fix the lint issues and add screenshots. But it's in a shape to review. I am working at limited capacity today.

[parasharrajat]

Sure, I will take a look. Thanks

[melvin-bot]

@JmillsExpensify, @mananjadhav, @luacmartins, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

[parasharrajat]

PR is in review. @mananjadhav Could you please mark this ready?

[luacmartins]

@parasharrajat it seems like the PR is still in draft 🤔 Am I missing something?

[luacmartins]

Ah just saw this comment above. Yea, let's address the failing checks and mark it as ready for review

[mananjadhav]

I am working on the checklist. There are too many lint errors. Trivial ones are fixed, but some would need more time to update methods and then test.

[luacmartins]

@mananjadhav I think we can address the remaining ESLint errors in a follow up. They seem unrelated to your PR and some of them can get quite tricky.

[mananjadhav]

Okay, how do we then ignore these? Will it affect other PRs? Because otherwise the PR is ready, left with just screenshots.

[luacmartins]

I think we can just merge it as is. It'll only affect other PRs touching those files. We should create a follow up PR to address those issues though, but I think it's cleaner that way to not distract from the main point of the current PR.