-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Reset last visible message on failure of attachment upload #22342
Reset last visible message on failure of attachment upload #22342
Conversation
@rushatgabhane 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] |
@rushatgabhane Friendly bump to review the PR |
Reviewer Checklist
Screenshots/VideosWeb69b97565-0589-4a61-83b8-d21f992dc1ee.mp4Mobile Web - ChromeWhatsApp.Video.2023-07-10.at.11.35.41.mp4Mobile Web - SafariScreen.Recording.2023-07-10.at.11.42.04.movDesktopScreen.Recording.2023-07-10.at.11.12.44.moviOSScreen.Recording.2023-07-10.at.11.38.58.movAndroidScreen.Recording.2023-07-10.at.11.27.17.mov |
Offline and online behavior tests well |
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.
@techievivek LGTM
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 a quick question around lastReadTime
key in failureData.
src/libs/actions/Report.js
Outdated
@@ -294,7 +295,16 @@ function addActions(reportID, text = '', file) { | |||
}, | |||
]; | |||
|
|||
const failureReport = { | |||
lastMessageText: prevVisibleMessageText, | |||
lastReadTime: currentTime, |
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.
Do we need to set the lastReadTime
in failureData? Curious to learn what purpose does it serve 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.
Thanks for pointing this out. Actually, lastReadTime is not required. It is a redundant code. Sorry about that. I have removed it and tested it across 3 platforms and it works well. I will test with the remaining platforms as well and update the code.
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.
Looks good.
@techievivek looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not an emergency merge, all the tests were passing. |
✋ 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/techievivek in version: 1.3.39-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.39-11 🚀
|
{ | ||
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, | ||
value: failureReport, | ||
}, |
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 caused a regression. If we send a message after the attachment and delete that message the LHN would appear again as [Attachment]
. Not exactly a bug from this PR but it caused the inconsistency as previously seeing [Attachment]
in LHN was the expected results.
@rushatgabhane @techievivek
Details
The changes include adding failureReport to failureData so that if there is a failure in API.write operation, it will use the previous visible message text.
The changes are in
addActions
function so that this would work for bothaddAttachment
andaddComment
Fixed Issues
$ #19913
PROPOSAL: #19913 (comment)
Test
Offline tests
The sidebar will continue to show this status as long as it is offline.
QA Steps
Same as above
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
Android/Native
21_andr-native-fix.mp4
Android/Chrome
22_andr-chrome-fix.mp4
iOS/Native
23_ios-native-fix.mp4
Web/Safari
26_mac-safari-web.mp4
Web/Chrome
25_web-chrome.mp4
MacOS/Desktop
27_mac-desktop.mp4
Android/Native Offline
28_andr-native-offline.mp4