Closed lanitochka17 closed 4 days ago
Triggered auto assignment to @puneetlath (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.
@puneetlath 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
We think that this bug might be related to #vip-vsb
Workspace - Workspace profile page becomes grayed out when go offline
Pending fields are not cleared in success data. https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/libs/actions/Policy/Policy.ts#L1596-L1602 https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/libs/actions/Policy/Policy.ts#L1673-L1677
Clear pending fields in success data for generalSettings
& description
also.
Workspace profile page becomes grayed out when go offline
We add these pendingFields in optimistic data https://github.com/Expensify/App/blob/659011e347acee51f320580a1841ce34ec6c1292/src/libs/actions/Policy/Policy.ts#L1600-L1601 But we do not remove them in Success data https://github.com/Expensify/App/blob/659011e347acee51f320580a1841ce34ec6c1292/src/libs/actions/Policy/Policy.ts#L1600-L1601
Actually, there's no need to include these lines because we're not waiting for a backend response for them. The backend response here doesnโt return a description or general settings, so we can safely remove these two lines. https://github.com/Expensify/App/blob/659011e347acee51f320580a1841ce34ec6c1292/src/libs/actions/Policy/Policy.ts#L1600-L1601
Job added to Upwork: https://www.upwork.com/jobs/~0182138e49511ea4ac
Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External
)
Edited by proposal-police: This proposal was edited at 2024-08-14 11:16:16 UTC.
Workspace profile page becomes grayed out when user goes offline
In this PR, we plan to just add generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION
, but description: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
is also added.
And these pendingFields are not cleared in successData
and failureData
:
https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/libs/actions/Policy/Policy.ts#L1673-L1677 leads to the bug.
generalSettings: null,
description: null,
and
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
pendingAction: null,
pendingFields: {
autoReporting: null,
approvalMode: null,
reimbursementChoice: null,
generalSettings: null,
description: null,
},
},
},
After this PR, we will use separately pending field outputCurrency
and address
instead of using generalSettings
. So we need to add these pending to:
https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/libs/actions/Policy/Policy.ts#L1596
address: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD
and then also clear these in successData
and failureData
.
We can remove all the pendingFields: https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/libs/actions/Policy/Policy.ts#L1596-L1602
<OfflineWithFeedback pendingAction={policy?.pendingFields?.generalSettings ?? policy?.pendingAction}>
Similar logic should applied to:
https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/pages/workspace/WorkspaceProfilePage.tsx#L214 https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/pages/workspace/WorkspaceProfilePage.tsx#L232 https://github.com/Expensify/App/blob/5d57ae84466a52dd25b12e9b0e76eda26281ae96/src/pages/workspace/WorkspaceProfilePage.tsx#L252
@shubham1206agra, just to note, the changes in this PR should be handled there only, or we can hold off until that PR is merged. It has nothing to do with this issue. The general solution will be the same, to add missing pending fields. The pending fields were intentionally added in this PR, so I think we shouldn't remove them.
@puneetlath, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
๐ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐ธ
@puneetlath, @shubham1206agra 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
@shubham1206agra thoughts on the proposals?
@puneetlath, @shubham1206agra Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!
@Krishna2323's proposal looks good.
๐๐๐ C+ reviewed
Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
@shubham1206agra Can you check my proposal? It will work in the latest code changes in main branch.
I think @Krishna2323 proposal and https://github.com/Expensify/App/issues/47260#issuecomment-2288493961 is fine with me.
With the changes in the selected proposal, the policy name is not grey out when creating policy in offline. Because now, we use policy?.pendingFields?.name
as the pending action for the policy name:
Btw, as mentioned in @Krishna2323 's comment:
The general solution will be the same, to add missing pending fields
So I think he can handle the details when creating PR. So that solution looks good.
@puneetlath @shubham1206agra this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@puneetlath, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Waiting on assignment from @puneetlath
โ ๏ธ 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.
@shubham1206agra just a quick update: we no longer use generalsettings.
The only thing left in the pending fields is description
. I've mentioned in my proposal that we should remove it.
https://github.com/Expensify/App/blob/9ddca5c4ac8168ad36da570192a6e4ef10820e67/src/libs/actions/Policy/Policy.ts#L1632
In the backend response, we donโt check for description
. According to the Offline_UX, weโre using the Optimistic Without Feedback Pattern, which means we shouldnโt change the UI in offline mode since we're not waiting for server confirmation.
cc:@puneetlath
@Nodebrute Thank you for the input. But your reasoning is wrong. We are using pattern B here. We should keep the description greyed out while creating the workspace. So, we will proceed with @Krishna2323's proposal as usual. And I will ask the deploy blocker to be marked as closed.
@shubham1206agra Thanks for clearing that up ๐
@puneetlath I think you wrote in the wrong issue
๐ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐ธ
๐ฃ @shubham1206agra ๐ An offer has been automatically sent to your Upwork account for the Reviewer role ๐ Thanks for contributing to the Expensify app!
๐ฃ @Krishna2323 ๐ 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 ๐
@trjExpensify Can you add label for payment on 12th September?
@puneetlath has actioned on that, I've added the Awaiting payment
label as well now.
I've paid @Krishna2323.
@shubham1206agra can you please complete the checklist?
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:
Ok great. All paid. Thanks everyone!
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: 9.0.19-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: N/A Email or phone of affected tester (no customers): gocemate+a919@gmail.com Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
Workspace profile page should not be grayed out since the workspace was created in online mode
Actual Result:
Workspace profile page becomes grayed out when user goes offline
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/ef924aba-a998-4be2-b32c-66346c3aec02
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @shubham1206agra