[HOLD for payment 2022-09-16] [$250] The file extension shows up twice in the file name reported by @Tushu17

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open website/mWeb.
2. Go to chat > Send a jpeg/pdf attachment..
3. See the name of attachment in preview.

Expected Result:

Extension shouldn't get repeated.

Actual Result:

It shows up twice.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Mobile Web

Version Number: 1.1.91-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:


Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @sonialiap (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[theTrozen77]

Hi, I have been able to reproduce this and fix it successfully.
return {fileName: splitFileName[0], fileExtension};
The filename returned was not correct previously.
Thankyou

[Puneet-here]

Proposal

There are two extensions because we aren't removing the extension from file name and we are adding the extension here too

App/src/components/AttachmentModal.js

Line 241 in d5b1c82

trailingText={fileExtension ? `.${fileExtension}` : ''}

The above proposal doesn't work great when a file name has periods in it. It will take the text before the period so the file name will be incomplete.
We can use slice(0, -1).join('.') after split. This way we will get the whole file name without extension then we just have to use that instead of the one with the extension.

[melvin-bot]

@sonialiap Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sonialiap]

Triaging, can confirm described behavior

[melvin-bot]

Triggered auto assignment to @johnmlee101 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[johnmlee101]

Opening for external, seems like a very easy fix based off of the two proposals already

[melvin-bot]

Triggered auto assignment to @arielgreen (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[theTrozen77]

Proposal

There are two extensions because we aren't removing the extension from file name and we are adding the extension here too

App/src/components/AttachmentModal.js

Line 241 in d5b1c82

trailingText={fileExtension ? `.${fileExtension}` : ''}

The above proposal doesn't work great when a file name has periods in it. It will take the text before the period so the file name will be incomplete. We can use slice(0, -1).join('.') after split. This way we will get the whole file name without extension then we just have to use that instead of the one with the extension.

I believe when Puneet is suggesting here might be an extension to this issue and could be a new ticket. However, my proposal above completely fixes the actual issue here which is that the extension are being repeated twice.

[Puneet-here]

It's not an extension, your proposal fixes the issue but breaks the file name when there are periods in it, if a solution breaks something then it's not a good solution

[sobitneupane]

Proposal
Reason:
splitExtensionFromFileName() function returns fileName as it is without removing the extension.

App/src/components/AttachmentModal.js

Lines 115 to 120 in 479ca44

	splitExtensionFromFileName(fullFileName) {
	const fileName = fullFileName.trim();
	const splitFileName = fileName.split('.');
	const fileExtension = splitFileName.pop();
	return {fileName, fileExtension};
	}

Solution:
As fileExtension is already popped from splitFileName, we can join splitFileName with '.' to get fileName without extension.

    splitExtensionFromFileName(fullFileName) {
        const fileName = fullFileName.trim();
        const splitFileName = fileName.split('.');
        const fileExtension = splitFileName.pop();
-      return {fileName, fileExtension};
+        return {fileName: splitFileName.join('.'), fileExtension};
    }

Output:

[varshamb]

Proposal

In AttachementModal, splitExtensionFromFileName() returns file name with extension.

We should return this instead -
const fileNameWithoutExtension = fileName.substring(0, fileName.lastIndexOf('.')) || fileName;

I think this code more readable.

App/src/components/AttachmentModal.js

Lines 115 to 120 in 479ca44

	splitExtensionFromFileName(fullFileName) {
	const fileName = fullFileName.trim();
	const splitFileName = fileName.split('.');
	const fileExtension = splitFileName.pop();
	return {fileName, fileExtension};
	}

[arielgreen]

https://www.upwork.com/jobs/~01c7f6606ce3547f5f

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

[melvin-bot]

Triggered auto assignment to @Luke9389 (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[parasharrajat]

Regression caused by #10118.

@sobitneupane 's proposal looks good to me.

cc: @Luke9389

🎀 👀 🎀 C+ reviewed

[Luke9389]

I agree that we should go with @sobitneupane's proposal. Go ahead and create a PR @sobitneupane.
Thank you @sobitneupane, @Puneet-here, @varshamb, @theTrozen77 for your proposals!

@Puneet-here your proposal would work, but I like that @sobitneupane's fixes the problem at the source (the splitExtensionFromFileName function).

@theTrozen77 & @Puneet-here, please try to refrain from openly criticizing each others' proposals. If a proposal is chosen and you disagree with the result, that's the time to talk about the flaws you see.

[Puneet-here]

please try to refrain from openly criticizing each others' proposals. If a proposal is chosen and you disagree with the result, that's the time to talk about the flaws you see.

Cool, will remember next time.

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.98-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove extension from fileName #10839

If no regressions arise, payment will be issued on 2022-09-16. 🎊

[arielgreen]

@sobitneupane @Tushu17 @parasharrajat Please check your Upwork inboxes!

[Luke9389]

just waiting on payment to close this out melvin.

[melvin-bot]

@parasharrajat, @arielgreen, @Luke9389 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[parasharrajat]

Bump @arielgreen, this is awaiting payment for a long time. Can we please remove the Reviewing label so that CM gets daily notifications?

[arielgreen]

thanks @parasharrajat resent

[arielgreen]

Paid.