Attribute automated actions to Concierge in NewDot - #76302
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@chuckdries 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] |
Reviewer Checklist
Screenshots/Videos
|
|
@JmillsExpensify you able to sign off on this one? |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Attributing changes to Concierge, similar to what we do in OldDot. Might've been an oversight when this was implemented in NewDot.
This wasn't an oversight, it was a specific product design/initiative. The strategy has changed a different direction now, so fine with the changes based on new product scope.
|
ok finally have all the tests passing, one more @chuckdries |
chuckdries
left a comment
There was a problem hiding this comment.
Third time's the charm!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/chuckdries in version: 9.2.74-0 🚀
|
|
This PR failing because of a regression issue: #77163 1.mp4 |
|
This PR is failing because of issue ##77185 The issue is reproducible in: Web IOS Uploading 524541728-b454baf3-f93b-4126-9658-6448003c9e7a.mp4… |
|
This PR is failing because of issue #hhttps://github.com//issues/77188 The issue is reproducible in: Web IOS Android |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.74-12 🚀
|
| // - Automatic approvals/forwards via workspace rules | ||
| // - Automatic payments via workspace rules | ||
| if (wasSubmittedViaHarvesting || (wasAutomatic && actionName !== CONST.REPORT.ACTIONS.TYPE.IOU) || (wasAutomatic && isPayment)) { | ||
| return CONST.ACCOUNT_ID.CONCIERGE; |
There was a problem hiding this comment.
We missed the case when messages are grouped. We need to use this function to determine message grouping too. See #77163
| let unavailableTranslation = ''; | ||
|
|
||
| Onyx.connect({ | ||
| Onyx.connectWithoutView({ |
There was a problem hiding this comment.
@justinpersaud @chuckdries These changes are wrong for the onyx connections. We've been working for months to try and fix this, and the changes here basically ignore all the work that has been done.
The name of the method should be the biggest hint that this is wrong. All of these onyx subscriptions ARE connected to methods used by views. There are open issues to refactor these properly, so the ideal approach would be to refactor them (rather than just changing the method name), but at the least, it would have even been better to leave them alone and ignore the ESLint warnings. That way, they will still stand out to the person that will refactor them later.
An ideal refactoring means:
- Finding all the methods referring to the Onyx value
- Update them to have the Onyx value passed as a parameter to the function
- Remove the
Onyx.connect()reference completely
I would like to ask that you revert the changes here to use connectWithoutView(), please.

Explanation of Change
Attributing changes to Concierge, similar to what we do in OldDot. Might've been an oversight when this was implemented in NewDot.
Fixed Issues
Relates to https://github.com/Expensify/Expensify/issues/566447
Tests
Offline tests
QA Steps
For the steps below, you can use any historical report if you have one. You don't need to create a new one, but you can if can't find one.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari