-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor Edit and Delete Report Comment #9532
Conversation
Looks like you modified Instead, all new API commands should use API.js, and follow our guidelines for writing new API commands. Unsure if your change is okay? Drop a note in #expensify-open-source! |
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…port-edit-comment-refactor
…om:Expensify/App into paulogasparsv-report-edit-comment-refactor
Added some commits so dismissing my review here.
@Gonals looks like this was merged without passing tests. 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. |
Sitting here with @marcaaron. This was good to go! |
Thanks @Gonals @marcaaron!!! |
Updated the last 2 QA tests I've added for checking the lastMessageText to match the latest changes (before it was checking while offline, updated to checking it only when online) |
@PauloGasparSv Why? Those are the QA steps if they are failing QA let them fail... |
Because the changes in one your latest commits changed a bit the way the lastMessageText is updated @marcaaron! Weren't the changes intentional? |
Let's put them back in and create follow up issues if we need to and they do not pass QA? I doubt they are blockers for the deploy but maybe polish steps. |
Oh but I didn't remove them, just changed 2 lines of the test steps because this logic was removed. Changed both "Test editing the latest message" and "Test deleting the latest message" to be tested without toggling the app to offline because now the API is the on responsible for updating that. Doesn't make sense to me to have tests that will knowingly and intentionally fail in QA. If that's the plan, why did we remove that logic in this P.R. and not in a follow up? |
Details
Update the api calls to the new
UpdateComment
andDeleteComment
api commands. Also implement the Optimistic with Feedback pattern B to these api calls.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/211601
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
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)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
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)Test Steps
Run QA steps first then the following:
Test edit comment error
Note: Simulate failure by throwing a test message instead of allowing the edit
throw new ExpException("Could not update message!", 400);
Test delete comment error
throw new ExpException("Could not delete message!", 400);
QA Steps
Test greyed editing comment
Test strike-through deleted comment
Test greyed deleted Attachment Comment
Test editing the latest message
Test deleting the latest message
Screenshots
Web
Mobile Web
Desktop
iOS
Android