[OldDot Rules Migration] Tag rules

[marcaaron]

Part of the OldDot Rules Migration project

Main issue: https://github.com/Expensify/Expensify/issues/413886

Feature Description

High Level Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.noull64ot4cc

Detailed Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.a5rfjg2ykxym

Manual Test Steps

TBD

Automated Tests

TBD

[BrtqKr]

Hey, I'm from SWM I'd like to take over this ticket

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[marcaaron]

In progress! We'll get to this after the next couple of PRs are merged (likely on Monday).

[melvin-bot]

@JmillsExpensify, @marcaaron, @BrtqKr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[BrtqKr]

Sorry for the delay, I got sick - we've decided to wire this up after @WojtekBoman is done with one of his tasks since there's a major shared part between both of those tickets. I'll let you know whenever it's done, but in general finishing this off afterwards shouldn't take much time

[BrtqKr]

We've found a couple of discrepancies between the docs and the back-end along the way. Right now I'm sending something like that and I'm getting 200 with no errors

But, I'm not receiving any onyx data in response

@marcaaron, is there anything I need to do for that to respond as expected?

Before that, I tried to send params as described in the design doc, but there have been errors regarding the format

[marcaaron]

This is when calling SetPolicyTagApprover?

I'm not sure if I see where we'd return any modified data in the response, but can look into it. It should be Ok for now since if it succeeds then the optimistic data would be correct.

I tried to send params as described in the design doc, but there have been errors regarding the format

Can you clarify what you mean by this? What did you send and what errors did you see?

[marcaaron]

We should pass policyID, tagName and approver not email.

[marcaaron]

It's weird because I was sending it this way before changing it to email. Right now, it's going through with 200 and everything seems correct

To clarify, the API will allow you to do this without an email or approver param because we default the approver value to an empty string which would effectively remove the approver.

but I'm not able to fetch it in openPolicyTagsPage. Maybe some data is missing in that read action?

I didn't really understand the question here, but I think you are saying that we need to include the rules data somewhere - e.g. either in OpenApp or later when calling OpenPolicyTagsPage? Let me know if that's correct. There are a few cases like this popping up where we missed adding some data in the backend.

[BrtqKr]

I didn't really understand the question here, but I think you are saying that we need to include the rules data somewhere - e.g. either in OpenApp or later when calling OpenPolicyTagsPage? Let me know if that's correct. There are a few cases like this popping up where we missed adding some data in the backend.

yes, we're missing it in OpenPolicyTagsPage, so we cannot persist it properly even if the write is passing as expected

[marcaaron]

Ok, thanks for bringing that up! We're working on this today (should not need to block anything though).

[BrtqKr]

ok, I'll pass the whole thing for the review and we'll hold the merge until the back-end is deployed. It'd be nice to merge the expense reports before that because this PR is based on that, otherwise there will be unnecessary diff displaying.

[marcaaron]

This should be all unblocked now!

[rojiphil]

High Level Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.noull64ot4cc

Detailed Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.a5rfjg2ykxym

@marcaaron Can you please give me(rojiphil@gmail.com) access to these documents?

[marcaaron]

Done!

…

On Mon, Sep 9, 2024 at 5:27 PM Roji Philip ***@***.***> wrote: High Level Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.noull64ot4cc Detailed Section: https://docs.google.com/document/d/1oLr14YhL6Y0N5g4tbozdIIrFbybBlsRA0H9I8Wm--w8/edit#bookmark=id.a5rfjg2ykxym @marcaaron <https://github.com/marcaaron> Can you please give me( ***@***.***) access to these documents? — Reply to this email directly, view it on GitHub <#47016 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH3RC75JI74JXABJ4NJBAADZVZRIRAVCNFSM6AAAAABMFMNYWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZZGUZTIMJRGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- github.com/marcaaron

[melvin-bot]

⚠️ 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.

[melvin-bot]

⚠️ 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.

[rojiphil]

Note: The blockers mentioned here (i.e. related to categories and raised even before the PR got merged) and here (as clarified here) were false positives for the issue at hand here.

[rojiphil]

And I think this is due for payment shortly since the feature implementation for this issue has been in production for a week now

I do have a humble request here. Can we consider a 2x compensation for my C+ reviewer role as it took more time than usual due to intertwining with issues related to categories and also due to the related design challenges? Thanks

[marcaaron]

That definitely works for me if @JmillsExpensify has no objections. You did a great job on the PR review and were a big help!

[marcaaron]

@JmillsExpensify can you please help pay @rojiphil and then close this out after? It's done and just waiting for payment!

[rojiphil]

@JmillsExpensify A gentle bump on payment here. Thanks.

[JmillsExpensify]

Payment summary: $500 to @rojiphil for PR review and testing.

[rojiphil]

Payment summary: $250 to @rojiphil for PR review and testing.

@JmillsExpensify If you have no objections, are you fine to make it 2x as mentioned here and here

[JmillsExpensify]

Ah cool, I updated the payment summary.

[JmillsExpensify]

All paid out via Upwork!