-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Edit Task Front End #18672
Edit Task Front End #18672
Conversation
src/libs/actions/Task.js
Outdated
|
||
function beginEditingTask() { | ||
Onyx.merge(ONYXKEYS.TASK, {isEditing: true}); | ||
} | ||
|
||
/** Sets the task editing property in the store to false */ | ||
function endEditingTask() { | ||
Onyx.merge(ONYXKEYS.TASK, {isEditing: false}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we need to know if we are currently editing a task or not? There is another form we could use that we are currently using for title / description which is EDIT_TASK_FORM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the TaskAssigneeSelectorModal
I'm reusing it and want it to effectively do the same thing, just call a different function if we're editing / creating a new task. Alternatively I could instead do a check to see if a report exists on the TASK
onyx key, which I'm setting whenever we're on a task report page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, yeah this approach is probably better
Alternatively I could instead do a check to see if a report exists on the TASK onyx key, which I'm setting whenever we're on a task report page.
This might not work since the task won't always be stored in onyx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw this comment, updating now!
dc97fe6
to
b499ba5
Compare
@jasperhuangg 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] |
if (option.alternateText) { | ||
// Check to see if we're creating a new task | ||
// If there's no route params, we're creating a new task | ||
if (!props.route.params && option.alternateText) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the alternateText contain that is being checked here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternateText being passed in the email
we assign to assignee. It's just a check to make sure it's not malformed data.
src/libs/actions/Task.js
Outdated
...report, | ||
reportName: title || report.reportName, | ||
description: description || report.description, | ||
assignee: assignee || report.assignee, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we not have the title and just the report object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EditTask
is running individually when saving in the Title, Description, or Assignee page, so 2 of the 3 editable options will be blank when running the function.
@thienlnam yup! Looks like just some errors when there's not an assignee. Fixed that up, but do we want to have a different "Unassigned" state? Just have that text? |
Yeah, let's just do the text for now to keep it consistent |
df86d06
to
69d06ba
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@fedirjh was this when an assignee wasn't in contacts? |
@cdanwards Yes, I think we should cover that case so the app don’t crash. |
@fedirjh @thienlnam I updated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the getAssignee function so that it won't error when we're selecting an assignee that's not in our contacts, but it can lead to a UI state where there's nothing present, like the unassigned UI state.
I am not sure of that, I think this is similar to the issue with the create task when you select an assignee outside of your contact? I think both cases have the same fix .
@thienlnam Should we have an option to un-assign someone ?
@cdanwards another bug , The money request reports are included in share somewhere of create task, they don’t have a title , I think we should exclude them from share somewhere page
type: CONST.REPORT.MESSAGE.TYPE.TEXT, | ||
style: 'normal', | ||
text: ' edited this task', |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
if (reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.TASKEDITED) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fine for now ? as the edit report action is not being showing in task report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the edit reportAction should not be shown on the task report at all
33e8c6d
to
daf9ae1
Compare
Reviewer Checklist
Screenshots/VideosWebWeb.Task.Edit.movMobile Web - ChromeEdit.Task.mWeb.Chome.movEdit.Task.mWeb.Chome.2.movMobile Web - SafariEdit.Task.mWeb.Safari.movDesktopEdit.Task.Desktop.moviOSEdit.Task.IOS.movAndroidEdit.Task.Android.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! Nice work all
@thienlnam looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
All tests had passed |
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.14-0 🚀
|
@@ -1429,6 +1429,7 @@ function buildOptimisticCreatedReportAction(ownerEmail) { | |||
actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, | |||
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, | |||
actorAccountID: currentUserAccountID, | |||
actorEmail: currentUserEmail, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line caused regression - #19010
As actorEmail
is added to created action, now it meets this condition:
App/src/libs/ReportActionsUtils.js
Line 160 in 46d27b8
return currentAction.actorEmail === previousAction.actorEmail; |
which allows grouping with new incoming message.
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
let assigneeChatReportID; | ||
if (assignee && assignee !== report.assignee) { | ||
assigneeChatReportID = ReportUtils.getChatByParticipants([assignee]).reportID; | ||
optimisticAssigneeAddComment = ReportUtils.buildOptimisticTaskCommentReportAction(report.reportID, reportName, assignee, `Assigned a task to you: ${reportName}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this action while in DM will duplicate the Task preview which caused this issue #19126
function getShareDestination(reportID, reports, personalDetails) { | ||
const report = lodashGet(reports, `report_${reportID}`, {}); | ||
return { | ||
icons: ReportUtils.getIcons(report, personalDetails), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ Coming from #21582
The task can be created inside the money request thread. So we should get the correct avatar by passing the isPayer
from ReportUtils.isIOUReport
result.
return { | ||
icons: ReportUtils.getIcons(report, personalDetails), | ||
displayName: ReportUtils.getReportName(report), | ||
subtitle: ReportUtils.getChatRoomSubtitle(report), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Coming from #22296
ReportUtils.getChatRoomSubtitle
returns an empty string for 1 to 1 DM's, which resulted in no subtitle for task assignee
We get the correct subtitle for those chats (email/phone number of another participant) from report.participants[0]
now
This issue was not handled in this PR. Issue: #27296 It might have been out of the scope of this PR. |
value: { | ||
reportName, | ||
description: description || report.description, | ||
assignee: assignee || report.assignee, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set errorFields: null
for report in optimisticData
, otherwise it will cause this bug #33782
Details
Fixed Issues
$ #16856
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Low.Res.Desktop.mov
Offline tests
Same as Tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Low.Res.Desktop.mov
Mobile Web - Chrome
Low.Res.Android.mov
Mobile Web - Safari
Low.Res.iOS.mov
Desktop
Low.Res.Desktop.1.mov
iOS
Low.Res.iOS.mov
Android
Low.Res.Android.mov