Fix: Android - Error attempting to download an attachment #25742
Conversation
|
@aimane-chnaif could you help with the PR too? |
sure |
|
@aimane-chnaif ICYMI. Please help review the PR. |
|
@dukenv0307 can you share example file name after fix? |
|
@aimane-chnaif
New ( Please note that all illegal characters, ":" in this case, are replaced by " ")
|
It looks weird. Can we have better naming which represents time in different format? |
|
@aimane-chnaif You mean the alternative solution in my proposal: #25491 (comment) When you review it, I thought we all agreed that we should go for the option: Replacing illegal characters in the file name |
can you share example file name after applying your alternative solution. |
|
@aimane-chnaif it looks like this: rn_image_picker_lib_temp_0015beab-6aa4-4971-984b-8f786e45812d-2023-08-24 102904 |
|
I don't have strong opinion on file naming as long as they don't have invalid characters. old: rn_image_picker_lib_temp_0015beab-6aa4-4971-984b-8f786e45812d-2023-08-24 10:16:10.200 I think new2 is better |
|
@dukenv0307 one more thing: we should add ms as well to avoid regression of #23094 in case user click download button many times quickly. As they will happen in a second |
I see, will add ms if we opt for the date-formating solution |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.mp4Mobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mp4 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
There was a problem hiding this comment.
I've highlighted a few areas related to code quality that we might consider for future reference or even as potential enhancements. These suggestions are offered in the spirit of collaboration and shared learning.
I appreciate the efforts of everyone involved in this Pull Request, and I believe that these discussions contribute to our collective growth as a team. If anyone has thoughts or questions about my observations, I'm more than happy to engage in a dialogue.
| @@ -1198,6 +1198,7 @@ const CONST = { | |||
| TIME_STARTS_01: /^01:\d{2} [AP]M$/, | |||
| TIME_FORMAT: /^\d{2}:\d{2} [AP]M$/, | |||
| DATE_TIME_FORMAT: /^\d{2}-\d{2} \d{2}:\d{2} [AP]M$/, | |||
| ILLEGAL_FILENAME_CHARACTERS: /\/|<|>|\*|"|:|\?|\\|\|/g, | |||
There was a problem hiding this comment.
Consider using the list operator [] for single characters in Regex, as it's more concise and doesn't require as many escape sequences:
// Instead of:
ILLEGAL_FILENAME_CHARACTERS: /\/|<|>|\*|"|:|\?|\\|\|/g,
// Use:
ILLEGAL_FILENAME_CHARACTERS: /[\/<>*":?\\|]/g,This makes the code cleaner and aligns with common practices for handling individual characters.
| const expectedFileName = `image-${DateUtils.getDBTime()}.jpg`; | ||
| expect(actualFileName).toEqual(expectedFileName); | ||
| expect(actualFileName).toEqual(expectedFileName.replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_')); |
There was a problem hiding this comment.
I noticed that the test is using part of the same logic as the implementation, specifically the .replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_'). This means it's essentially testing the implementation against itself, which can mask errors and lead to false positives.
A more robust approach would be to test against a static expectation, as shown below:
it('should append current time to the end of the file name', () => {
const fixedTime = '2023-08-25 13:56:12.119';
jest.spyOn(DateUtils, 'getDBTime').mockReturnValue(fixedTime);
const actualFileName = FileUtils.appendTimeToFileName('image.jpg');
const expectedFileName = 'image-2023-08-25 13_56_12.119.jpg'; // Static expectation
expect(actualFileName).toEqual(expectedFileName);
});In this revised version, the expected file name is hardcoded, and a mock ensures that the time appended to the file name is known and fixed. This makes the test more robust and reliable.
By removing the .replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_'), we ensure that if someone modifies the Regex in a way that introduces an error, the test has a chance to catch it. This change aligns the test more closely with the principle of testing the expected outcome rather than replicating the implementation logic.
There was a problem hiding this comment.
Thanks for suggestions. Agree to use static values in automated test. I also had this in mind but we used dynamic value - DateUtils.getDBTime() already before this PR, I hadn't raised this concern.
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.58-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.58-5 🚀
|
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
Details
Replace illegal characters in the filename when downloading an attachment Android
Fixed Issues
$ #25491
PROPOSAL: #25491 (comment)
Tests
Offline tests
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
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.3.mp4