fix: Update task title without opening the report #32249
Conversation
ac16d9e to
63a5380
Compare
|
@cubuspl42 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] |
| const allReports: Record<string, Report> = {}; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT, | ||
| callback: (report, key) => { | ||
| if (!key || !report) { | ||
| return; | ||
| } | ||
| const reportID = CollectionUtils.extractCollectionItemID(key); | ||
| allReports[reportID] = report; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This basically line-to-line duplicate of this block, and maybe others.
There was a problem hiding this comment.
Yup. I once had a somewhat similar question, and this was the response from @tgolen:
I haven’t seen duplicate connect()s be a problem yet, so that’s what I would do.
I may not be aware, is there a way to reuse the Onyx.connect?
There was a problem hiding this comment.
I don't know, maybe ReportUtils.getReportById or something like that? It seems werid to just copy-paste this because there "hasn't been a problem yet"...
There was a problem hiding this comment.
@tgolen what would you say? Can we reuse utils inside other utils instead of using Onyx.connect?
There was a problem hiding this comment.
That is a very nuanced topic, so I apologize in advance for the long comment, but I want you to hear my thinking.
-
There is no technical problem with having the same
connect()in two different libs. The only "problem" is that it is repetitive code, which is a somewhat valid argument. However, let's take that logic all the way to the grand conclusion. If we only intend to have oneconnect()for every key, then that basically results in some kind of file that contains all theconnect()calls for every key. This then creates an odd in-memory type store of everything that Onyx already has in memory. This is just a strange place to end up in. -
Having duplicate
connect()s in the files that need them allows them to be custom-tailored for fast lookups or custom maps depending on what data is being accessed. For example, rather than doing a.filter()or.map()every time the data needs accessed, theconnect()callback should store the data in an efficient way so that it can be easily and quickly accessed likereports[reportID]and there is no need to filter or map. This allows for code to be much more optimized, rather than kept in a very general and slow method (because it has to account for different libs wanting to access it in different ways).
There was a problem hiding this comment.
Thanks, this makes sense in terms of productivity vs clean code.
Based on the comment above, should we be good to go with this approach @cubuspl42?
There was a problem hiding this comment.
This doesn't make sense, at least to me.
The only "problem" is that it is repetitive code, which is a somewhat valid argument
Code duplication is an industry-standard problem with industry-standard solutions.
Even more precisely, even the duplication of data access code is industry-standard, this has been present since the old days of hand-crafted MPA DAOs written in PHP, Java, or whatever. The only twist is the data is reactive here, changing as the time flows.
We've discussed the "big picture", the general case, but coming down to our case, the only argument I can hear for not de-duplicating this is "this might be optimized in the future, so let's keep the duplication", which falls into the YAGNI category for me.
Having said all this, it's not a blocker for me, as I'm kind of busy and can't afford discussing this too much.
Reviewer Checklist
Screenshots/VideosWebtask-title-clipboard-web.mp4Mobile Web - Chrometask-title-clipboard-android-web-compressed.mp4Mobile Web - Safaritask-title-clipboard-ios-web.mp4DesktopiOStask-title-clipboard-ios.mp4Androidtask-title-clipboard-android-compressed.mp4 |
|
@thienlnam Can you please get this reviewed soon? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| const isReportPreviewAction = ReportActionsUtils.isReportPreviewAction(reportAction); | ||
| const message = _.last(lodashGet(reportAction, 'message', [{}])); | ||
| const reportID = lodashGet(reportAction, 'originalMessage.taskReportID', '').toString(); | ||
| const messageHtml = isTaskAction || isCreateTaskAction ? Task.getTaskReportActionMessage(reportAction.actionName, reportID, isCreateTaskAction) : lodashGet(message, 'html', ''); | ||
| const messageHtml = isTaskAction ? Task.getTaskReportActionMessage(reportAction) : lodashGet(message, 'html', ''); |
There was a problem hiding this comment.
Hi, it looks like this method was moved to TaskUtils but this reference wasn't updated. It results in the following webpack warning on main and it will probably cause the app to crash:
[web-server] WARNING in ./src/pages/home/report/ContextMenu/ContextMenuActions.js 277:37-68
[web-server] export 'getTaskReportActionMessage' (imported as 'Task') was not found in '@userActions/Task' (possible exports: canModifyTask, cancelTask, clearOutTaskInfo, clearOutTaskInfoAndNavigate, clearTaskErrors, completeTask, createTaskAndNavigate, dismissModalAndClearOutTaskInfo, editTask, editTaskAssignee, getAssignee, getShareDestination, getTaskAssigneeAccountID, reopenTask, setAssigneeValue, setDescriptionValue, setDetailsValue, setShareDestinationValue, setTaskReport, setTitleValue)
[web-server] @ ./src/pages/home/report/ContextMenu/BaseReportActionContextMenu.js 24:0-78 39:8-40 65:44-62
[web-server] @ ./src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js 11:0-72 311:38-65
[web-server] @ ./src/Expensify.js 43:0-108 209:39-69
[web-server] @ ./src/App.js 23:0-36 55:97-106
[web-server] @ ./index.js 6:0-28 11:9-12
|
@paultsimura Could you please spin up another GH to fix the webpack error? |
|
@thienlnam sure, will do tomorrow |
|
The fix PR is ready for review: #32774. It'd be best if we could get it reviewed and merged before this one is deployed to staging. |
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.11-0 🚀
|
This comment was marked as resolved.
This comment was marked as resolved.
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.11-25 🚀
|
| function getTaskCreatedMessage(reportAction: OnyxEntry<ReportAction>) { | ||
| const taskReportID = reportAction?.childReportID ?? ''; | ||
| const taskTitle = getTaskTitle(taskReportID, reportAction?.childReportName); | ||
| return Localize.translateLocal('task.messages.created', {title: taskTitle}); | ||
| } |
There was a problem hiding this comment.
If taskTitle is falsy we should have returned an empty string as it does not make sense to return task create message with no task title i.e. task for .
Returning empty string enables other other parts of the app to detect failure in getting task created message so we can fallback to other messages.
(Coming from #35002)
Details
In this PR, 2 issues are fixed:
Fixed Issues
$ #29035
PROPOSAL: #29035 (comment)
Tests
Same as QA
Offline tests
The user must be online to receive new tasks and task title updates.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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-compressed.mp4
Android: mWeb Chrome
chrome-compressed.mp4
iOS: Native
ios-compressed.mp4
iOS: mWeb Safari
safari-compressed.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4