-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: the download's key for files with the same filename #5534
Conversation
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 see sanitizeString
makes an extra work for URLs, although the function doesn't imply that, and the change to sanitizeLikeString
seems to just replace unwanted characters with _
.
const sanitizeString = (value: string) => { |
You might want to change serverUrlParsedAsPath
to sanitizeLikeString
as well, just for the sake of semantics
const serverUrlParsedAsPath = (serverURL: string) => `${sanitizeString(serverURL)}/`; |
At least I can't see a reason to do that on
serverUrl
.
I'm also missing a clear description of why sanitizeLikeString
is better than sanitizeString
and a string as example of what was causing the issue.
The error occurred when there were more than 1 file with the same filename, for example:
The |
I see now. You're calling the url to the attachment on backend |
5f03f6a
to
a017891
Compare
The solution is simple, the explanation was unnecessarily complex. |
Proposed changes
The error occurred when there were more than 1 file with the same filename, for example:
https://open.rocket.chat/file-upload/ChJDEKZhJHc359rm8/Screenshot%202024-02-14%20at%2019.54.07.png
https://open.rocket.chat/file-upload/4roCoCiHzsxzZzFXv/Screenshot%202024-02-14%20at%2019.54.07.png
The
mediaDownloadKey
function is responsible for determining the key related to the download object, but it was only capturing the filename from the URL. Thus, when there were 2 files with the same filename, the key was associated with only one, causing the download of the other to appear as if it was also in progress, when in fact, it was referencing the download of the first file. To fix this, I decided not to separate only the filename to use as the key for the object. Instead, I used the complete link as the object key, thus differentiating between each file.Issue(s)
How to test or reproduce
Screenshots
Before
Screen.Recording.2024-02-01.at.22.21.43.mov
After
Screen.Recording.2024-02-01.at.22.20.32.mov
Types of changes
Checklist
Further comments
CORE-55