[OldDot Rules Migration] Tag rules #48325

BrtqKr · 2024-08-30T11:41:05Z

Holding on:

[OldDot Rules Migration] Expense report rules #47468 (to avoid conflicts)
https://github.com/Expensify/Auth/pull/12370 (deploy)

Details

Fixed Issues

$ #47016
$ #48977
PROPOSAL:

Tests

Go to Profile => workspaces => select a workspace => tags
Select an existing tag, or create a new one
Change approver
Verify that selecting the same person, removes them as an approver
Verify that there are correct system messages being sent to the admin channel

Verify that disabling rules in more features, disables approver as well

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Screen.Recording.2024-09-05.at.12.26.16.mov

Android: mWeb Chrome

Screen.Recording.2024-09-05.at.12.31.59.mov

iOS: Native

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-05.at.11.58.45.mp4

iOS: mWeb Safari

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-05.at.12.05.07.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-09-05.at.10.41.15.mov

MacOS: Desktop

Screen.Recording.2024-09-05.at.12.36.56.mov

melvin-bot · 2024-08-30T11:41:09Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

marcaaron · 2024-09-06T19:07:21Z

The PRs we were waiting on are now deployed or merged so we can take this off HOLD.

BrtqKr · 2024-09-09T09:27:38Z

@marcaaron, it seems to be in a different field than I initially expected, but I've applied the changes and it's working. I believe we can pass it on for review.

melvin-bot · 2024-09-09T09:27:52Z

@rojiphil Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

rojiphil · 2024-09-10T03:14:28Z

@BrtqKr I will review the PR today. Meanwhile, can you please resolve the conflicts? Thanks

rojiphil

@BrtqKr I have a few observations as mentioned below but the overall code looks super good though:

Once an approver is selected, we do not allow deselecting the approver to set it to none. I think we need to support this considering that OldDot supports this.
The test steps in PR mentions this:

Verify that disabling tags in more features, disables approver as well

Neither do I see any implementation for this nor do I see any reference to this in test videos. Have we missed implementing this? Also, what would disables approver mean here? I think it will help QA if we can further break down the test steps

BrtqKr · 2024-09-10T08:25:07Z

@rojiphil

I've added the removal option
That's an error in the description, I meant rules and that's already implemented

                    {policy?.areRulesEnabled && (
                        <>
                            <View style={[styles.mh5, styles.mv3, styles.pt3, styles.borderTop]}>
                                <Text style={[styles.textNormal, styles.textStrong, styles.mv3]}>{translate('workspace.tags.tagRules')}</Text>
                            </View>
                            <OfflineWithFeedback pendingAction={policy.pendingFields?.rules}>
                                <MenuItemWithTopDescription
                                    title={tagApprover ?? ''}
                                    description={translate(`workspace.tags.approverDescription`)}
                                    onPress={navigateToEditTagApprover}
                                    shouldShowRightIcon
                                />
                            </OfflineWithFeedback>
                        </>
                    )}

rojiphil

2. That's an error in the description, I meant rules and that's already implemented

@BrtqKr Thanks for the changes. The removal feature works fine but the feature of disabling approvers does not seem to be implemented yet as we do for category approver rules. Here is a test video demonstrating this. Please have a look. Thanks

48325-disabledapprover.mp4

BrtqKr · 2024-09-10T20:56:42Z

if I understand correctly the condition should probably be uniform and it's been missing for the categories. It's already applied with the remaining fixes

marcaaron · 2024-09-16T20:23:56Z

@BrtqKr Let us know when this is ready for another review. Seems @rojiphil has some of the checklist left to do, but I think they are waiting for the latest comments to get resolved. Let me know if there's anything I can do to help unblock!

BrtqKr · 2024-09-17T14:28:03Z

@marcaaron @rojiphil
I'm not sure if this suggestion makes sense. If I understand correctly:

The main issue is that after renaming the category its content is moved to the new entry in the dictionary
Since the rules are operating on the category name as a key, there is no relation between them, so the approver is disappearing

But...adding any id-based condition for disable (I'm not sure what kind of id you mean here, because neither category nor tag has one) without having them saved with a proper relation is not going to work, because at some point we're not going to be able to differentiate what the rule was supposed to apply to.

It's not a matter of the optimistic update, even a single change to the category name or tag name breaks the relation between the rules and categories. The only things I can do at this point is to:

Allow to edit a category once(because we have a previous name)and then completely disable it
Completely disable rename in the offline mode
Try to somehow relate everything in the actions, but this is going to be really messy and would cause bugs.

marcaaron · 2024-09-17T21:20:59Z

Here’s what I am going to propose for now:

When category or tag name changes call getCategoryApproverRule() or getTagApproverRule()
Update the rule with the new name. They are linked by the name (which is also unique) so I assume this should work even when we are offline and should fix the issue of the "approver" not transferring.
Let’s punt the conversation about “pending” stuff for now and also scrap the current changes - essentially not providing feedback for the pending stuff. I think it did not get enough consideration in the design phase. I also think that we need to re-think the design. Looking at what we have currently and I’m not sure I like it. But, it's ok, not having the feedback is also not a huge deal for now. So, let's just remove those parts and keep what we like?

At some point later we can explore:

Better offline handling.
Different data structure (perhaps we can consolidate the related data so it is easier to update and set errors and also switch to objects instead of arrays for "rules" since they are harder to work with).

BrtqKr · 2024-09-18T00:30:04Z

I've removed the offline pattern and added category/tag renaming in the rules. I believe this would be everything

Screen.Recording.2024-09-18.at.01.57.14.mov

rojiphil · 2024-09-18T10:01:35Z

src/libs/actions/Policy/Policy.ts

@@ -2578,7 +2578,10 @@ function createWorkspaceFromIOUPayment(iouReport: OnyxEntry<Report>): WorkspaceF
 });

 // We need to move the report preview action from the DM to the workspace chat.
- const reportPreview = ReportActionsUtils.getParentReportAction(iouReport);
+ const parentReport = ReportAcionsConnection.getAllReportActions()?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReport.parentReportID}`];


@BrtqKr Not sure if I am missing something here but why did we have to create ReportAcionsConnection when we already have a utility ReportActionsUtils.getAllReportActions?

because we have a new eslint rule, which is forcing us to fix those things when we edit files containing deprecated code

well, not exactly eslint rule, but more like a check included inside of the one for eslint

@BrtqKr Ah! I see. So, the deprecated function is getParentReportAction but is there a need to create ReportAcionsConnection.getAllReportActions() when ReportActionsUtils.getAllReportActions gives us the same result?

This is something I'll need to discuss separately, but from what I understand the main issue is that the rule has been prepared with components in mind, so it's expecting some kind of action to be taken anyway. I was thinking about the possible benefits of having a connection in one place instead of having logic split between different files, but as I said earlier, I'll discuss it in more detail with someone who has more context.

This doesn't feel like a blocker.

This is something I'll need to discuss separately

getParentReportAction has been used in many places and I agree that this needs a separate discussion. Since this is not at all related to this issue, I would prefer not to include the changes for this in this PR. As and when there is a conclusion on the way forward, we can add the code changes. Anyway, I would leave the final decision on this to @marcaaron.

If we are leaving the code as it is, let us correct the spelling ReportAcionsConnection.

rojiphil · 2024-09-18T12:05:50Z

package-lock.json

@@ -27979,22 +27979,6 @@
 "jest": "bin/jest.js"
 }
 },
- "node_modules/jest-expo/node_modules/@babel/code-frame": {


@BrtqKr Did this change got introduced unintended as these seem unrelated to our current scope here?

I haven't modified anything in package.json and I'm using the appropriate node version. The change was introduced right after merging main to this branch and reinstalling the packages, so I'd assume it wasn't indended, but it's correct.

This also doesn't seem like a blocker.

rojiphil · 2024-09-18T12:38:06Z

Otherwise the code looks good and the feature works well.
I have left a couple of comments. Once we address them, I can do the final testing.
Looks very close to getting this done.

rojiphil

@BrtqKr I have left few suggestions related to test cases

Let us add the below mentioned steps from 5th step onward in the existing test steps.

Select an approver again
Change the name of the tag
Verify that the approver does not change.

Also, let us add the following so that QA can test categories too

Repeat the above mentioned steps for categories and verify.

Let us remove the following text from steps as QA will get confused:

Verify that disabling rules in more features, disables approver as well

@marcaaron Other than this comment, the code changes related to tag/category approver feature LGTM and works well too and I have completed the checklist.
Over to you for review.

marcaaron

LGTM

marcaaron · 2024-09-19T20:15:11Z

src/languages/es.ts

@@ -3233,6 +3233,8 @@ export default {
 importedFromAccountingSoftware: 'Etiquetas importadas desde',
 glCode: 'Código de Libro Mayor',
 updateGLCodeFailureMessage: 'Se produjo un error al actualizar el código de Libro Mayor. Por favor, inténtelo nuevamente.',
+ tagRules: 'Reglas de etiquetas',
+ approverDescription: 'Aprobador',


NAB we could probably move this to the general since it is used twice.

OSBotify · 2024-09-19T20:28:33Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-09-19T23:54:25Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.39-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

WojtekBoman and others added 4 commits August 28, 2024 21:03

Add WorkspaceMembersSelectionList

53828c7

add interface for tag edit

374f298

wire up the tags

299856e

wire up action

4a3bfcc

marcaaron changed the title ~~[OldDot Rules Migration] Tag rules~~ [HOLD] [OldDot Rules Migration] Tag rules Sep 5, 2024

marcaaron changed the title ~~[HOLD] [OldDot Rules Migration] Tag rules~~ [OldDot Rules Migration] Tag rules Sep 6, 2024

marcaaron assigned BrtqKr Sep 6, 2024

BrtqKr added 3 commits September 9, 2024 10:22

Merge remote-tracking branch 'origin/main' into brtqkr/47016-tag-rules

d64e7b6

cleanup, modify action

10dbc8e

change translations

e9dbc5c

BrtqKr marked this pull request as ready for review September 9, 2024 09:27

BrtqKr requested a review from a team as a code owner September 9, 2024 09:27

melvin-bot bot requested a review from rojiphil September 9, 2024 09:27

melvin-bot bot removed the request for review from a team September 9, 2024 09:27

rojiphil reviewed Sep 10, 2024

View reviewed changes

BrtqKr added 2 commits September 10, 2024 09:54

Merge remote-tracking branch 'origin/main' into brtqkr/47016-tag-rules

99d973f

add remove approver option

78df971

BrtqKr requested a review from rojiphil September 10, 2024 08:27

rojiphil reviewed Sep 10, 2024

View reviewed changes

marcaaron self-requested a review September 10, 2024 20:31

BrtqKr added 3 commits September 10, 2024 22:46

disable when workflows are disabled

7f792c8

add approver conditon

6163fe5

add approver conditon to category settings

bd3fdba

Merge remote-tracking branch 'origin/main' into brtqkr/47016-tag-rules

d7e902a

BrtqKr added 6 commits September 18, 2024 00:50

update category rule when renaming category

fe5f31c

update tag rule when renaming tag

bc4eed8

remove pending action pattern for rules

6fc0879

Merge remote-tracking branch 'origin/main' into brtqkr/47016-tag-rules

69d919a

fix onyx check

08cf2a2

remove pending pattern from type

b32ed64

BrtqKr requested a review from marcaaron September 18, 2024 00:30

BrtqKr added 4 commits September 18, 2024 02:37

run prettier

71c8ef8

cleanup

669e692

cleanup

4fccbb1

remove withOnyx

2fdea82

rojiphil reviewed Sep 18, 2024

View reviewed changes

BrtqKr added 3 commits September 18, 2024 16:49

Merge remote-tracking branch 'origin/main' into brtqkr/47016-tag-rules

d91e3f3

run prettier

4810e33

cleanup

2ba3191

rojiphil approved these changes Sep 19, 2024

View reviewed changes

rename connection

e06551c

marcaaron approved these changes Sep 19, 2024

View reviewed changes

marcaaron merged commit 0045eae into Expensify:main Sep 19, 2024
19 checks passed

github-actions bot mentioned this pull request Sep 19, 2024

Deploy Checklist: New Expensify 2024-09-19 #49505

Open

26 tasks

nkdengineer mentioned this pull request Sep 20, 2024

Task – App crashes when click on assignee in existing task #49514

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OldDot Rules Migration] Tag rules #48325

[OldDot Rules Migration] Tag rules #48325

BrtqKr commented Aug 30, 2024 •

edited

Loading

melvin-bot bot commented Aug 30, 2024

marcaaron commented Sep 6, 2024

BrtqKr commented Sep 9, 2024

melvin-bot bot commented Sep 9, 2024

rojiphil commented Sep 10, 2024

rojiphil left a comment

BrtqKr commented Sep 10, 2024

rojiphil left a comment •

edited

Loading

BrtqKr commented Sep 10, 2024 •

edited

Loading

marcaaron commented Sep 16, 2024

BrtqKr commented Sep 17, 2024

marcaaron commented Sep 17, 2024 •

edited

Loading

BrtqKr commented Sep 18, 2024

rojiphil Sep 18, 2024

BrtqKr Sep 18, 2024 •

edited

Loading

rojiphil Sep 18, 2024 •

edited

Loading

BrtqKr Sep 18, 2024

marcaaron Sep 18, 2024

rojiphil Sep 19, 2024

rojiphil Sep 18, 2024

BrtqKr Sep 18, 2024

marcaaron Sep 18, 2024

rojiphil commented Sep 18, 2024

rojiphil left a comment

marcaaron left a comment

marcaaron Sep 19, 2024

OSBotify commented Sep 19, 2024

OSBotify commented Sep 19, 2024

[OldDot Rules Migration] Tag rules #48325

[OldDot Rules Migration] Tag rules #48325

Conversation

BrtqKr commented Aug 30, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 30, 2024

marcaaron commented Sep 6, 2024

BrtqKr commented Sep 9, 2024

melvin-bot bot commented Sep 9, 2024

rojiphil commented Sep 10, 2024

rojiphil left a comment

Choose a reason for hiding this comment

BrtqKr commented Sep 10, 2024

rojiphil left a comment • edited Loading

Choose a reason for hiding this comment

BrtqKr commented Sep 10, 2024 • edited Loading

marcaaron commented Sep 16, 2024

BrtqKr commented Sep 17, 2024

marcaaron commented Sep 17, 2024 • edited Loading

BrtqKr commented Sep 18, 2024

Choose a reason for hiding this comment

BrtqKr Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

rojiphil Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rojiphil commented Sep 18, 2024

rojiphil left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Sep 19, 2024

OSBotify commented Sep 19, 2024

BrtqKr commented Aug 30, 2024 •

edited

Loading

rojiphil left a comment •

edited

Loading

BrtqKr commented Sep 10, 2024 •

edited

Loading

marcaaron commented Sep 17, 2024 •

edited

Loading

BrtqKr Sep 18, 2024 •

edited

Loading

rojiphil Sep 18, 2024 •

edited

Loading