Tails

#1 Updated by sajolida 2018-09-19 01:15:48

• Assignee changed from sajolida to intrigeri
• QA Check set to Ready for QA
• Feature Branch set to web/15964-pass-through-r

Here is a branch.

intrigeri: Do you want to review this as part of Fundraising? Otherwise I’ll look for somebody else.

#2 Updated by intrigeri 2018-09-19 08:01:56

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

> intrigeri: Do you want to review this as part of Fundraising?

Sure.

Questions/comments:

• I don’t know how to test this.
• Do we need to leave the console.log debugging output in place when merging this? I suspect not.
• Regarding element selection:
  • It feels wrong to hard-code the list of elements we want to change and query them by name, while we have a class (return-url) to select them. The good news is that iterating over all elements with that class (presumably with getElementsByClassName()) will also fix the next potential problem:
  • document.getElementsByName("return")[0] suggests we’re only updating the URL for the first such element, while I see we have 5 of them in the HTML. Is there an easy way for me to confirm that all such URLs are updated?

#3 Updated by sajolida 2018-09-21 20:46:54

• Status changed from Confirmed to In Progress

Applied in changeset commit:38730560b3794959e02052ec29fcd0df1c5d03ba.

#4 Updated by sajolida 2018-09-21 21:46:47

• File dom.png added
• File post.png added
• Assignee changed from sajolida to intrigeri
• QA Check changed from Info Needed to Dev Needed

> Sure.

Thanks!

> * I don’t know how to test this.

1. Visit /donate on a local version of the website.

2. Add a “r” parameter to the URL. For example, I visited:

file:///home/amnesia/Persistent/Tor%20Browser/master/donate.html?r=something

3. Open the Firefox developers tools (F12).

4. Go to the “Network” tab.

5. Click on the “Donate” button.

6. Analyze the POST parameters sent to PayPal.

Your “r” should be in the “cancel_return” and “return” parameters.

See screenshot in attachment.

7. Repeat without adding an “r” and it should not be sent to PayPal.

> * Do we need to leave the console.log debugging output in place when merging this? I suspect not.

Nope :) Fixed in 114a97f1fa.

> It feels wrong to hard-code the list of elements we want to change and query them by name […]

Done in c8fc4cab77.

> document.getElementsByName("return")[0] suggests we’re only updating the URL for the first such element, while I see we have 5 of them in the HTML.x

The last 4 are used in the non-JS version of the page so they don’t really matter…

> Is there an easy way for me to confirm that all such URLs are updated?

… but you can check that in the Inspector of the Firefox Developers Tools.

See in attachment for where to find them :)

#5 Updated by sajolida 2018-09-22 21:17:42

• QA Check changed from Dev Needed to Ready for QA

#6 Updated by intrigeri 2018-10-09 09:14:22

• % Done changed from 0 to 60

All this makes sense, thanks! Code review passes, will now test.

#7 Updated by intrigeri 2018-10-09 09:40:51

• Status changed from In Progress to Resolved
• Assignee deleted (intrigeri)
• % Done changed from 60 to 100
• QA Check changed from Ready for QA to Pass

Works great, merged :)