[HOLD for payment 2024-11-14] [$250] Wrong validation message when sending invoice #49286

m-natarajan · 2024-09-16T17:29:54Z

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

Version Number: 9.0.35-7
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @dubielzyk-expensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1726459401827979

Action Performed:

Click green (+) button
Send invoice
Enter $200
Choose recipient
Fill in company name (Dubby Co)
Fill in company website (www.dubbyco.com)

Expected Result:

Expected www.dubbyco.com to be a valid website address

Actual Result:

Gives error saying "Please enter a valid website using lowercase characters"

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

CleanShot.2024-09-16.at.14.00.05.mp4

View all open jobs on GitHub

Recording.548.mp4

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021835835067982464212
Upwork Job ID: 1835835067982464212
Last Price Increase: 2024-09-24
Automatic offers:

ahmedGaber93 | Reviewer | 104377926
Krishna2323 | Contributor | 104377928

Issue Owner

Current Issue Owner: @ahmedGaber93

melvin-bot · 2024-09-16T17:30:07Z

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

nkdengineer · 2024-09-16T17:33:01Z

Edited by proposal-police: This proposal was edited at 2024-09-19 08:21:17 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Wrong validation message when sending invoice

What is the root cause of that problem?

Here, we are considering that a valid website must include http/https/ftp in the URL, so a website like www.example.com would be an invalid website.

What changes do you think we should make in order to solve the problem?

We should add the https:// prefix by using the Str.sanitizeURL function when validating the website and submitting the form here and here

      if (!ValidationUtils.isValidWebsite(Str.sanitizeURL(values.companyWebsite))) {
          errors.companyWebsite = translate('bankAccount.error.website');
      }

App/src/pages/iou/request/step/IOURequestStepCompanyInfo.tsx

Line 52 in 37bf6b2

if (!ValidationUtils.isValidWebsite(values.companyWebsite)) {

      IOU.sendInvoice(currentUserPersonalDetails.accountID, transaction, report, undefined, policy, policyTags, policyCategories, values.companyName, Str.sanitizeURL(values.companyWebsite));

App/src/pages/iou/request/step/IOURequestStepCompanyInfo.tsx

Line 72 in 37bf6b2

    
           IOU.sendInvoice(currentUserPersonalDetails.accountID, transaction, report, undefined, policy, policyTags, policyCategories, values.companyName, values.companyWebsite);

OPTIONAL: We should also do the same in several places within the app that use the isValidWebsite function like here and here

What alternative solutions did you explore? (Optional)

github-actions · 2024-09-16T18:06:26Z

Jon Øvrebø Dubielzyk Your proposal will be dismissed because you did not follow the proposal template.

Krishna2323 · 2024-09-16T18:28:32Z

Edited by proposal-police: This proposal was edited at 2024-09-19 15:55:37 UTC.

Proposal 

Please re-state the problem that we are trying to solve in this issue.

Wrong validation message for website when sending invoice.

What is the root cause of that problem?

We are using isValidWebsite for validation which requires http/https/ftp URL scheme.

App/src/libs/ValidationUtils.ts

Lines 236 to 243 in 6c82111

    
           /** 
        
            * Similar to backend, checks whether a website has a valid URL or not. 
        
            * http/https/ftp URL scheme required. 
        
            */ 
        
           function isValidWebsite(url: string): boolean { 
        
               const isLowerCase = url === url.toLowerCase(); 
        
               return new RegExp(`^${Url.URL_REGEX_WITH_REQUIRED_PROTOCOL}$`, 'i').test(url) && isLowerCase; 
        
           }

App/src/pages/iou/request/step/IOURequestStepCompanyInfo.tsx

Line 52 in 12779f1

if (!ValidationUtils.isValidWebsite(values.companyWebsite)) {

What changes do you think we should make in order to solve the problem? 

If we want a default value, we can get it using getDefaultCompanyWebsite and pass it to defaultValue prop.

What alternative solutions did you explore? (Optional)

We can update yourCompanyWebsiteNote to also include the website example https://www.expensify.com.
We can also update error.website to also include the website example https://www.expensify.com.
We can use prefixCharacter prop to add https:// prefix.

What alternative solutions did you explore? (Optional 2)

We should create a new util function similar to isValidWebsite but in that function we will use Url.URL_REGEX or URL_WEBSITE_REGEX. Or we can update isValidWebsite to use those regex.

function isValidWebsite(url: string): boolean {
    const isLowerCase = url === url.toLowerCase();
    return new RegExp(`^${Url.URL_WEBSITE_REGEX}$`, 'i').test(url) && isLowerCase;
}

We will replace the use of ValidationUtils.isValidWebsite with the new util function if choose to create a new one.
We also need to create a new function similar to snitizeURL to add http when the url does not has a protocol.
Then we would need to update these functions to use the new snitizeURL function to modify the url before sending it ti the backend.

App/src/libs/actions/IOU.ts

Line 3638 in 7947b8e

function sendInvoice(

App/src/libs/actions/BankAccounts.ts

Line 401 in 7947b8e

function updateCompanyInformationForBankAccount(bankAccountID: number, params: Partial<CompanyStepProps>, policyID: string, isConfirmPage: boolean) {
Then we can use the new util function in IOURequestStepCompanyInfo and also in WorkspaceInvoicingDetailsWebsite if required.
Updating updateCompanyInformationForBankAccount is optional.

Result

melvin-bot · 2024-09-17T00:16:04Z

Job added to Upwork: https://www.upwork.com/jobs/~021835835067982464212

melvin-bot · 2024-09-17T00:16:09Z

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

bikramiter · 2024-09-17T11:52:45Z

App/src/libs/ValidationUtils.ts

Lines 236 to 243 in 3c6526d

    
           /** 
        
            * Similar to backend, checks whether a website has a valid URL or not. 
        
            * http/https/ftp URL scheme required. 
        
            */ 
        
           function isValidWebsite(url: string): boolean { 
        
               const isLowerCase = url === url.toLowerCase(); 
        
               return new RegExp(`^${Url.URL_REGEX_WITH_REQUIRED_PROTOCOL}$`, 'i').test(url) && isLowerCase; 
        
           }

Note: As mentioned in the comment, this might need the corresponding scheme handling on the backend as well.

ahmedGaber93 · 2024-09-17T13:03:40Z

this might need the corresponding scheme handling on the backend as well.

I think it should be done in FE side, not BE. by adding the URL scheme if not found before sending to BE

ahmedGaber93 · 2024-09-18T06:23:51Z

Based on the issue Slack conversation, We have three possible options to fix this issue:

allow user to type URL without https:// and add it in behalf of the user before sending to BE
keep the current validation and just displaying the correct error message e.g. url must start with https://
add a default value https:// prefix on the input like this

Just to confirm, the expected result is option one? @kadiealexander @dubielzyk-expensify

dubielzyk-expensify · 2024-09-19T03:48:29Z

My personal take is 1 and 3 should be done here, but I dunno the technical or security implications of 1. I'd at least say 3 should be done.

Keen to hear what @trjExpensify @JmillsExpensify @Expensify/design thinks

shawnborton · 2024-09-19T04:23:49Z

Yeah that makes sense to me. Or even just do 2 and 3 might be easy without needing back end changes?

dubielzyk-expensify · 2024-09-19T05:16:20Z

Yeah that also works. Feels a bit inelegant that we don't validate it properly though, but perhaps it's the right priority at this time 👍

ahmedGaber93 · 2024-09-19T06:58:26Z

I think We can use option one without any backend change, we will allow the user to type www.dubbyco.com and send the complete format https://www.dubbyco.com to the backend.

We already have a function for adding the missing https:// protocol in frontend sanitizeURL

shawnborton · 2024-09-19T06:59:13Z

Oh nice, that's great then!

nkdengineer · 2024-09-19T08:22:05Z

updated proposal following this comment here

Krishna2323 · 2024-09-19T10:05:06Z

@ahmedGaber93, I believe we used prefix in the app somewhere in the past, maybe on the business details page, and adding it also helps the user understand that only https:// is accepted. It provides a visual reminder that only https:// is allowed, offering instant feedback while maintaining control over the protocol.

WDYT @shawnborton @dubielzyk-expensify?

dubielzyk-expensify · 2024-09-19T10:06:58Z

Sure. Let's go with option 1 and 3 then unless @shawnborton thinks otherwise. If it's all front-end stuff then chuck the prefix in and validate it to be a URL 👍

ahmedGaber93 · 2024-09-19T10:37:03Z

@dubielzyk-expensify the option 3 is already used on company website on bank account steps, but it has two cases

A. if user email is not on private domain, the default value will be prefix https://
B. if user email is on private domain, the default value will be generated website url https://www.privatedomain.com

Should we use the same UX behavior here for consistency or just use case A? And if we will use the both cases, should we also add default value for company name like case B (if user email is on private domain, company name default value will be privatedomain)?

shawnborton · 2024-09-19T12:38:10Z

I'm down with a combo of 1 and 3. As for the latest question... I feel like the default value of https:// would be enough?

ahmedGaber93 · 2024-09-19T13:29:01Z

Thanks, that's great.

trjExpensify · 2024-09-19T15:42:35Z

I'm not sure I'm following the 1 and 3 combo really. If we do 3, https:// is prefixed in the field and can't be removed, so wouldn't doing 3 only be enough?

If @ahmedGaber93 is saying in the bank account flow we populate your private domain email address into the field already, that kinda' makes sense to copy/paste over here in the invoice flow.

Krishna2323 · 2024-09-19T15:58:58Z

Proposal Updated

Updated main solution

davidcardoza · 2024-10-28T18:19:29Z

Waht's the latest here? @caitlinwhite1 reported the same issue today - https://expensify.slack.com/archives/C05LX9D6E07/p1730139071302859

ahmedGaber93 · 2024-10-28T18:21:51Z

PR in progress

melvin-bot · 2024-11-06T09:38:51Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot · 2024-11-06T09:53:24Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot · 2024-11-07T12:48:32Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-11-07T12:48:37Z

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

Wrong validation message when sending invoice #51545

If no regressions arise, payment will be issued on 2024-11-14. 🎊

For reference, here are some details about the assignees on this issue:

@ahmedGaber93 requires payment automatic offer (Reviewer)
@Krishna2323 requires payment automatic offer (Contributor)

melvin-bot · 2024-11-07T12:48:38Z

@ahmedGaber93 @kadiealexander The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

kadiealexander · 2024-11-15T03:01:33Z

Payouts due:

Contributor: $250 @Krishna2323 automatic offer
Reviewer: $250 @ahmedGaber93 automatic offer

Upwork job is here.

kadiealexander · 2024-11-15T03:01:59Z

@ahmedGaber93 please action the checklist, thanks!

ahmedGaber93 · 2024-11-15T12:03:06Z

BugZero Checklist:

[Contributor] Classify the bug:

Bug classification

Source of bug:

1a. Result of the original design (eg. a case wasn't considered)
1b. Mistake during implementation
1c. Backend bug
1z. Other: New feature

Where bug was reported:

2a. Reported on production
2b. Reported on staging (deploy blocker)
2c. Reported on both staging and production
2d. Reported on a PR
2z. Other:

Who reported the bug:

[Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

Link to comment: N/A. It is a new feature, we didn't support company website without https before
[Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

Link to discussion: N/A
[Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
[BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

Link to issue:

Regression Test Proposal

Precondition: you should have at latest one workspace has enabled "Invoices" feature

Go to Settings > workspaces > any workspace > More features
Enable Invoices

Test:

Click green (+) button
Send invoice
Enter $200
Choose recipient
Fill in company name
Fill in company website with a URL without https:// e.g. www.dubbyco.com
Verify the entered website is displayed as valid and can be saved

Do we agree 👍 or 👎

ahmedGaber93 · 2024-11-15T12:06:54Z

@ahmedGaber93 please action the #49286 (comment), thanks!

@kadiealexander Done!

melvin-bot · 2024-11-19T09:07:52Z

@marcaaron, @ahmedGaber93, @kadiealexander, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

ahmedGaber93 · 2024-11-19T09:12:02Z

@kadiealexander checklist is completed. Could you please issue the payment and close the issue?

melvin-bot · 2024-11-25T09:06:28Z

@marcaaron, @ahmedGaber93, @kadiealexander, @Krishna2323 Huh... This is 4 days overdue. Who can take care of this?

ahmedGaber93 · 2024-11-25T11:16:09Z

Bump @kadiealexander on slack to complete the payment and close this.

kadiealexander · 2024-11-26T06:05:05Z

https://github.com/Expensify/Expensify/issues/447811

m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 16, 2024

melvin-bot bot assigned kadiealexander Sep 16, 2024

kadiealexander added the External Added to denote the issue can be worked on by a contributor label Sep 17, 2024

melvin-bot bot changed the title ~~Wrong validation message when sending invoice From: @Jon Øvrebø Dubielzyk~~ [$250] Wrong validation message when sending invoice From: @Jon Øvrebø Dubielzyk Sep 17, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 17, 2024

melvin-bot bot assigned ahmedGaber93 Sep 17, 2024

kadiealexander added this to [#whatsnext] #billpay Sep 17, 2024

melvin-bot bot added the Weekly KSv2 label Oct 27, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 7, 2024

melvin-bot bot changed the title ~~[$250] Wrong validation message when sending invoice~~ [HOLD for payment 2024-11-14] [$250] Wrong validation message when sending invoice Nov 7, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Nov 7, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 14, 2024

melvin-bot bot added the Overdue label Nov 19, 2024

melvin-bot bot removed the Overdue label Nov 19, 2024

melvin-bot bot added the Overdue label Nov 25, 2024

melvin-bot bot removed the Overdue label Nov 25, 2024

kadiealexander closed this as completed Nov 26, 2024

github-project-automation bot moved this to Done in [#whatsnext] #billpay Nov 26, 2024

[HOLD for payment 2024-11-14] [$250] Wrong validation message when sending invoice #49286

[HOLD for payment 2024-11-14] [$250] Wrong validation message when sending invoice #49286

Comments

m-natarajan commented Sep 16, 2024 • edited by kadiealexander Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Sep 16, 2024

nkdengineer commented Sep 16, 2024 • edited by github-actions bot Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

github-actions bot commented Sep 16, 2024

Krishna2323 commented Sep 16, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

What alternative solutions did you explore? (Optional 2)

Result

melvin-bot bot commented Sep 17, 2024

melvin-bot bot commented Sep 17, 2024

bikramiter commented Sep 17, 2024

ahmedGaber93 commented Sep 17, 2024 • edited Loading

ahmedGaber93 commented Sep 18, 2024

dubielzyk-expensify commented Sep 19, 2024

shawnborton commented Sep 19, 2024

dubielzyk-expensify commented Sep 19, 2024

ahmedGaber93 commented Sep 19, 2024

shawnborton commented Sep 19, 2024

nkdengineer commented Sep 19, 2024

Krishna2323 commented Sep 19, 2024

dubielzyk-expensify commented Sep 19, 2024 • edited Loading

ahmedGaber93 commented Sep 19, 2024 • edited Loading

shawnborton commented Sep 19, 2024

ahmedGaber93 commented Sep 19, 2024

trjExpensify commented Sep 19, 2024

Krishna2323 commented Sep 19, 2024

Proposal Updated

davidcardoza commented Oct 28, 2024

ahmedGaber93 commented Oct 28, 2024

melvin-bot bot commented Nov 6, 2024

melvin-bot bot commented Nov 6, 2024

melvin-bot bot commented Nov 7, 2024

melvin-bot bot commented Nov 7, 2024

melvin-bot bot commented Nov 7, 2024

kadiealexander commented Nov 15, 2024 • edited Loading

kadiealexander commented Nov 15, 2024

ahmedGaber93 commented Nov 15, 2024

BugZero Checklist:

Regression Test Proposal

Precondition: you should have at latest one workspace has enabled "Invoices" feature

Test:

ahmedGaber93 commented Nov 15, 2024

melvin-bot bot commented Nov 19, 2024

ahmedGaber93 commented Nov 19, 2024

melvin-bot bot commented Nov 25, 2024

ahmedGaber93 commented Nov 25, 2024

kadiealexander commented Nov 26, 2024

m-natarajan commented Sep 16, 2024 •

edited by kadiealexander

Loading

nkdengineer commented Sep 16, 2024 •

edited by github-actions bot

Loading

Krishna2323 commented Sep 16, 2024 •

edited

Loading

Proposal 

What changes do you think we should make in order to solve the problem? 

ahmedGaber93 commented Sep 17, 2024 •

edited

Loading

dubielzyk-expensify commented Sep 19, 2024 •

edited

Loading

ahmedGaber93 commented Sep 19, 2024 •

edited

Loading

kadiealexander commented Nov 15, 2024 •

edited

Loading