Add validation to competition tab content to not allow relative URLs

[danieljames-dj]

There are cases where the competition tab content will have relative URLs instead of full URLs. In many cases, the relative URLs may not be intentional. To avoid those cases, added an error if the URL doesn't exist.

[FinnIckler]

I think this is a good idea, but wouldn't this really slow down saving the competition form? It takes seconds to timeout a request, so even with a couple of wrong links this would turn into a really slow request

[dunkOnIT]

Another option here might be to disable relative links entirely - perhaps a simpler approach to this, as organizers are probably trying to link to absolute URLs in most cases anyway?

[simonkellly]

Is a potential use case of relative urls to link to something like the registration tab, or a waiting list tab etc. from the same competition without having to change the tab content from a template being cloned?

[dunkOnIT]

Hmm that does make sense. I wonder if it would be possible to create some kind of variable like {competition-id} that people could use in their URL's? This would also fix issues with people hardcoding links to the competition that end up dead later on.

[danieljames-dj]

@FinnIckler This is applicable only for competition tab and not for competition form's markdown fields. Yes, it will slow down the competition tab but it will check only those URLs which doesn't have "http" prefix, and hence all the cases will be within WCA websites. So I guess this should be fine, please let me know if this doesn't makes sense to you, thank you.

I like idea of @dunkOnIT - disable relative links entirely. But as @simonkellly said, they might use it for linking registration tab or waiting list, or something like that.

But I am wondering if anybody will actually be using relative links in the competition tab. This can be investigated with help of WRT by querying the database. The reason why I wonder if it's actually used is because the relative URLs will get "https://worldcubeassociation.org/competitions" prefixed. So in case they want to add relative link to another tab, they have to add the relative link as "CompetitionId2024#1234-tabid" and I don't think somebody will do this instead will directly paste the full URL.

[danieljames-dj]

I had a quick discussion with @gregorbg over slack and he also agrees with @dunkOnIT's suggestion of not allowing relative URLs. I've implemented that now.

[gregorbg]

There's already a block competition.errors in the YML file, you should move this key there and drop the _error_message suffix.

[danieljames-dj]

Done

[gregorbg]

verify_full_urls should do, maybe I can be convinced to call it verify_if_full_urls. But you don't have to turn the method name into a whole English sentence 😉

[danieljames-dj]

Okay used verify_if_full_urls then :-)

[gregorbg]

Now that some time has passed, we've had more and more reports about URL errors. They lead me to doubt how useful this feature will be...

Most of the time, we get an issue where the Delegate/Organizer pastes a link containing the competition ID itself, for example when they want to link to a certain tab (ie "See the [Accomodation tab] for information on where to sleep!"), and these links break when the competition ID is updated.

Those links would continue to break, no matter whether we enforce them as https://worldcubeassociation.org/competitions/MyOutdatedCompetitionIDThatGotChanged2024/register or whether we enforce them as /competitions/MyOutdatedCompetitionIDThatGotChanged2024/register

Is there some use-case that I'm missing? What problem were we originally trying to solve with this PR?

[danieljames-dj]

One of the case which this PR is trying to solve is as follows:

Imagine the report is having following line in markdown editor:

[Test](test.com)

This is one of the error which got redirected to WST in past. The organizer is expecting that the hyperlink to "Test" will be "http://test.com" but actually it will be going to "http://worldcubeassociation.org/competitions/test.com".

This might block people who do intentionally - for example, if somebody gives hyperlink as "Comp2024/register" expecting that the remaining part of URL get prefixed correctly, then that won't work. But my claim/assumption is that at least 90% of users (who use hyperlink this way) will use this feature wrongly and hence cause trouble to WST.

[gregorbg]

Why is this limited to on: :update? If a new tab is created, this would currently allow relative URLs to "slip through".

[danieljames-dj]

I didn't think about that case. I just tried cloning a competition that has this error, the result was the competition got created, but the tab didn't get created and no error was thrown to the delegate. Will it be better if we allow to do this mistake during clone and stop them only during update?

[gregorbg]

That's... odd 😅
I mean, in the current scenario with your code as-is right now, of course the error won't get thrown to the Delegate. Because cloning makes it so that a clone of everything is created, meaning the on: :update wouldn't trigger during the cloning process.

[danieljames-dj]

I mean if I add :create, still the error won't be thrown to delegate. The cloning will happen without any issues, just that the tabs with error won't get created which might potentially create confusions among delegates why those didn't get created.

[danieljames-dj]

Currently since it's just :update, the faulty tabs are also getting created, so won't create any confusions to delegate.

[gregorbg]

Okay, we definitely need to look into why that happens. Because you just tested it with cloning. But if a Delegate is legitimately creating a fully new competition tab, then they would also be able to circumvent this filter, making it useless in at least ~30% of cases.