MBS-13884: Add ACUM to whitelist for other databases #3454
Conversation
dvirtz
changed the title
lib/MusicBrainz/Server/Data/URL.pm
Outdated
| @@ -20,6 +20,7 @@ my %URL_SPECIALIZATIONS = ( | |||
| # External links section | |||
| '45cat' => qr{^https?://(?:www\.)?45cat\.com/}i, | |||
| '45worlds' => qr{^https?://(?:www\.)?45worlds\.com/}i, | |||
| 'ACUM' => qr{^https?://nocs.acum.org.il/acumsitesearchdb/}i, | |||
There was a problem hiding this comment.
It feels like skipping acumsitesearchdb might be fine, but you might know better. You should escape the dots so they match just dots and not anything.
| return url.replace(/&.*/, '').replace('/version?', '/work?'); | ||
| }, | ||
| validate(url, id) { | ||
| const m = /^(https:\/\/)?nocs\.acum\.org\.il\/acumsitesearchdb\//i.test(url); |
There was a problem hiding this comment.
Normally the point of m is to extract a part of the URL you're going to compare against. It would make sense to either rename this to "isAcumLink" or whatnot, or actually extract the bits you want to compare against here already.
Also, don't do (https:\/\/)? here - clean the URL up to always have the same (probably https://?) as part of the clean method above (we do that for many URLs, just copy it). Then just use the expected clean format here.
There was a problem hiding this comment.
Done.
I did the match here in the initial implementation but the regex got too complicated with all the different supported entities.
| } | ||
| case LINK_TYPES.otherdatabases.label: | ||
| return { | ||
| result: /\/results\?(creatorid=I-\d+-\d|creatoripnnumber=\d+)$/.test(url), |
There was a problem hiding this comment.
Is this an actual page? It looks like a search URL, but I cannot see it since it still blocks me, so :)
There was a problem hiding this comment.
That's the page you get to when clicking a label name. Same for artist.
|
@reosarevok is there anything left for me to do? |
|
It might be fine now :) I'm hoping one of my teammates with proper access to the site can give this a last test - maybe @mwiencek can see it? |
|
Ok, @mwiencek could see it and sent me some screenshots. The search result pages are search results, even if you open them by clicking the name. That's common on libraries, and we still don't support linking to them since they're not pages for the entity, just searches. The work (and arrangement) pages seem fine. It's unclear if versions should be cleaned up to works - I understand they can and often have different ISWCs? Maybe we should support them as-is as well? Album is a bit weird but it seems a decent fit, so I guess we can have it too. |
|
The |
|
Ok, I guess since it goes straight to the ID, and you already wrote it, maybe it's still ok to have... not a huge fan, but I can see how it's halfway to a normal page that way, so :) |
Problem
MBS-13884
Solution
white list nocs.acum.org.il links as 'other database'
Testing
Tested adding ACUM links to artists, albums and works.