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

MBS-13572 / MBS-13573: Don't break on more LiveNation and Ticketmaster links #3253

Merged
merged 2 commits into from May 15, 2024

Conversation

reosarevok
Copy link
Member

@reosarevok reosarevok commented May 2, 2024

Implement MBS-13572 / MBS-13573

Problem

For LiveNation, I did not find links with _ nor - when implementing this originally, but clearly they exist, as the example in the ticket (now added to the test) shows. We were breaking these with the cleanup.

For Ticketmaster, there seems to be a second, less common type of links that can use all uppercase and lowercase letters in the IDs, which again I had not found when implementing this and was being rejected by the cleanup.

Solution

Just accept _ and - on LiveNation identifiers, and allow all of \w on Ticketmaster ones.

Testing

Added the previously breaking example links to the automated tests.

@reosarevok reosarevok changed the title MBS-13572: Support LiveNation links with _ and - MBS-13572: Don't break on more LiveNation and Ticketmaster links May 2, 2024
@reosarevok reosarevok changed the title MBS-13572: Don't break on more LiveNation and Ticketmaster links MBS-13572 / MBS-13573: Don't break on more LiveNation and Ticketmaster links May 2, 2024
@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label May 2, 2024
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

Just a minor optional suggestion for readability.
LGTMBDNT otherwise 🚢

root/static/scripts/edit/URLCleanup.js Outdated Show resolved Hide resolved
I did not find links with _ nor - when implementing this, but
clearly they exist. This expands the cleanup to support them.
Seems the original regex was too restrictive, since there is
a second type of Ticketmaster links that can include any letters
including lowercase (see example added to test).
This just accepts any \w in the IDs since restricting further
seems possibly problematic now.
@reosarevok reosarevok merged commit 70bd4df into metabrainz:master May 15, 2024
1 of 2 checks passed
@reosarevok reosarevok deleted the MBS-13572 branch May 15, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish URL handling
Projects
None yet
2 participants