Skip to content
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

Revert #651 #657

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Revert #651 #657

merged 1 commit into from
Feb 29, 2024

Conversation

aswin-s
Copy link
Contributor

@aswin-s aswin-s commented Feb 29, 2024

This reverts commit 83ae619 where we modified the URL_WEBSITE_REGEX to prevent excessive backtracking. This however caused regression Expensify/App#37472. So reverting the commit to unblock other PRs. Will raise a new PR with fix.

Fixed Issues

$ Expensify/App#37472

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
  2. What tests did you perform that validates your changed worked?

QA

  1. What does QA need to do to validate your changes?
  2. What areas to they need to test for regressions?

@aswin-s aswin-s requested a review from a team as a code owner February 29, 2024 09:12
@melvin-bot melvin-bot bot requested review from MonilBhavsar and removed request for a team February 29, 2024 09:12
@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Feb 29, 2024

@aswin-s could you please update description to link correct issue "$ GH_LINK"
Also add meaningful PR title
And update tests and QA sections

Copy link
Contributor

@MonilBhavsar MonilBhavsar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have mentioned this commit 17e8f75
Sorry, I couldn't get the context. The mentioned commit updates babel version

@aswin-s
Copy link
Contributor Author

aswin-s commented Feb 29, 2024

@aswin-s could you please update description to link correct issue "$ GH_LINK" Also add meaningful PR description And update tests and QA sections

Updated.

You have mentioned this commit 17e8f75 Sorry, I couldn't get the context. The mentioned commit updates babel version

Fixed the description. Message was auto added by merge commit. Also please hold until I hear back from @AndrewGable or @ntdiary. Not sure if I should revert the PR first or just raise a fix.

@ntdiary
Copy link

ntdiary commented Feb 29, 2024

I think we can revert the PR first. As for the new solution, it will require more time to investigate. :)

@MonilBhavsar
Copy link
Contributor

@aswin-s you might want to open a PR in App to update expensify-common version

@MonilBhavsar MonilBhavsar merged commit 77d0b15 into Expensify:main Feb 29, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants