Tails

#1 Updated by sajolida 2018-03-22 13:00:04

• Assignee changed from sajolida to intrigeri
• Priority changed from Normal to Elevated
• QA Check set to Ready for QA

intrigeri: Am I not calling this function properly or is it a bug in mirror-pool-dispatcher?

I’m surprised I didn’t test that when writing the new download page so maybe it broke since then…

Marking as Elevated as it’s probably a regression.

If that will take time to fix I can remove this function call temporarily.

#2 Updated by intrigeri 2018-03-22 13:19:21

• Assignee changed from intrigeri to sajolida
• QA Check changed from Ready for QA to Info Needed

> intrigeri: Am I not calling this function properly or is it a bug in mirror-pool-dispatcher?

Could you please point me to the code where you call this function?

#3 Updated by sajolida 2018-03-23 10:21:15

• Assignee changed from sajolida to intrigeri
• QA Check deleted (Info Needed)

git grep replaceUrlPrefixWithRandomMirror or wiki/src/install/inc/js/download.js line 219 and 223.

#4 Updated by intrigeri 2018-03-23 10:48:57

• Assignee changed from intrigeri to sajolida

Thanks!

You’re calling this function correctly, but that function does not support being called twice on the same page, and it’s run by wiki/src/templates/page.tmpl already. The code has no way to know what part of the URL is the prefix it should replace with a new mirror’s, all it does is replace as many chars as the original URL prefix is with the new mirror’s. I’m not sure what’s the best way to fix that:

• Ideally every call to transformURL / replaceUrlPrefixWithRandomMirror would take note of the selected mirror so the next calls know how many chars shall be replaced. No idea how to do that but I would be surprised it that was impossible.
• IIRC DAVE had code to handle this by checking if the current (already modified) URL starts with one of our mirrors’ URL prefix, switch it back to the original URL, and then call transformURL on that one… or something like this. I’m not sure if/how it handled the case when the previously picked mirror was removed from our pool in the meantime but perhaps we don’t care.

(In passing, “Affected tool” should be “Installation Assistant”, no?)

#5 Updated by sajolida 2018-03-26 19:33:01

• Assignee changed from sajolida to intrigeri
• Priority changed from Elevated to Normal
• Parent task set to Bug #12328
• QA Check set to Ready for QA

I wrote d7c3924ff5 to handle this by substituting HTML elements instead of writing more JavaScript. It’s not very elegant but probably the simplest in terms of the code I can write.

But I didn’t manage to run lib/js/mirror-dispatcher.js on my local build of the website and didn’t find instructions on how to do that. So I couldn’t test it fully, but it should work :)

Now I also understand why we didn’t spot this problem earlier: it’s not occurring on local builds, even if mirror-dispatcher.js is inserted.

NB:

• I’m making this a child of Bug #12328 (aka “WebExtension for DAVE”) and I’ll clock this work as such.
• I’m downgrading the priority as it’s not a regression in the end… That probably never worked.
• I’m keeping Affected tool: “Verification Extension”. It’s a big ambiguous because the code is in download.js which is the interface between “Verification Extension” and “Installation Assistant” but if we didn’t have the verification extension this bug wouldn’t exist.

#6 Updated by intrigeri 2018-03-27 14:54:00

• Feature Branch set to web/15443-mangled-urls-on-download-retry

#7 Updated by intrigeri 2018-03-27 15:08:23

• Status changed from Confirmed to In Progress

Applied in changeset commit:9149aa8b76576a4d35d142218de58dc6f73a5468.

#8 Updated by intrigeri 2018-03-27 15:09:00

• Assignee changed from intrigeri to sajolida

> I wrote d7c3924ff5 to handle this by substituting HTML elements instead of writing more JavaScript. It’s not very elegant but probably the simplest in terms of the code I can write.

OK, this does not look too crazy to me => merged.

I had to resolve a conflict with current master in some CSS, please check my conflict resolution.

> But I didn’t manage to run lib/js/mirror-dispatcher.js on my local build of the website and didn’t find instructions on how to do that. So I couldn’t test it fully, but it should work :)

OK, so please test now that it’s on our live website :)

You might also want to unfuzzy the PO files.

> * I’m making this a child of Bug #12328 (aka “WebExtension for DAVE”) and I’ll clock this work as such.

Oops, I did not clock this separately from my Foundations Team review work so far.

And once you close this ticket, I think you can close its parent as well \o/

#11 Updated by sajolida 2018-03-28 18:05:25

• Status changed from In Progress to Resolved
• Assignee deleted (sajolida)
• QA Check deleted (Ready for QA)