Fix: cannot change assignee after task created #23530
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dukenv0307
changed the title
|
@situchan Bump, Please help to review this PR |
situchan
reviewed
Aug 2, 2023
|
@dukenv0307 this is what I suggest. does it make sense? + const report = useMemo(() => {
+ if (!props.route.params || !props.route.params.reportID) {
+ return null;
+ }
+ return props.reports[`${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`];
+ }, [props.reports, props.route.params]);
...
// Check to see if we're editing a task and if so, update the assignee
- if (props.route.params.reportID && props.task.report.reportID === props.route.params.reportID) {
+ if (report) {
// There was an issue where sometimes a new assignee didn't have a DM thread
// This would cause the app to crash, so we need to make sure we have a DM thread
- Task.setAssigneeValue(option.login, option.accountID, props.task.shareDestination, OptionsListUtils.isCurrentUser(option));
+ Task.setAssigneeValue(option.login, option.accountID, props.route.params.reportID, OptionsListUtils.isCurrentUser(option));
// Pass through the selected assignee
- Task.editTaskAndNavigate(props.task.report, props.session.accountID, {
+ Task.editTaskAndNavigate(report, props.session.accountID, {
assignee: option.login,
assigneeAccountID: option.accountID,
});
}
};
return (
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
{({didScreenTransitionEnd, safeAreaPaddingBottomStyle}) => (
<>
<HeaderWithBackButton
title={props.translate('task.assignee')}
- onBackButtonPress={() => (lodashGet(props.task.report, 'isExistingTaskReport') ? Navigation.dismissModal() : Navigation.goBack(ROUTES.NEW_TASK))}
+ onBackButtonPress={() => (report ? Navigation.dismissModal() : Navigation.goBack(ROUTES.NEW_TASK))}
/> |
|
@situchan Thanks for suggestion, I just updated |
|
@dukenv0307 please pull main |
|
@situchan Updated |
situchan
reviewed
Aug 3, 2023
| if (!props.route.params || !props.route.params.reportID) { | ||
| return null; | ||
| } | ||
| return lodashGet(props.reports, `${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`, undefined); |
There was a problem hiding this comment.
I don't think lodashGet is necessary here as props.reports and props.route.params always exist
There was a problem hiding this comment.
@situchan Make sense. Thanks for your comment. I updated
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
@pecanoro Friendly bump, please help to review this PR, thanks |
|
situchan
approved these changes
Aug 8, 2023
|
@dukenv0307 @situchan So I tested it again and it's not working when assigning someone to the existing task after closing the new task, I am getting an error in the console as well:
|
|
I just tested again and no issue for me. |
|
Hmm, I am on the branch for sure, let me test again |
|
I also checked I am in the last commit:
|
|
Ok, it happens when you assign someone you have never talked to, if you assign someone for which you already have a report, it works fine: Monosnap.screencast.2023-08-08.15-09-09.mp4 |
|
ok, reproduced. Seems like different root cause which also happens on main. On this line: Line 422 in fdcae9f |
pecanoro
approved these changes
Aug 8, 2023
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
87 tasks
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.52-0 🚀
|
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.52-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
We should use props.report and reportID from route instead of props.task.report
Fixed Issues
$ #23556
PROPOSAL: #23556 (comment)
Tests
Offline tests
Same above
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)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)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
Web
Screen-Recording-2023-08-01-at-13.29.47.mp4
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
Screen-Recording-2023-08-01-at-16.36.17.mp4
Android
android.mov