Project

General

Profile

Feature #15964

Feature #14877: Donation campaign 2018

Pass-through the ?r= parameter to /donate/thanks and /donate/canceled

Added by sajolida about 1 month ago. Updated 11 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Fundraising
Target version:
Start date:
09/19/2018
Due date:
% Done:

100%

QA Check:
Pass
Feature Branch:
web/15964-pass-through-r
Type of work:
Website
Blueprint:
Starter:
Affected tool:

Description

To be able to know which donation incentives lead to more donations.

dom.png View (73.1 KB) sajolida, 09/21/2018 09:46 PM

post.png View (39.3 KB) sajolida, 09/21/2018 09:46 PM

Associated revisions

Revision 38730560 (diff)
Added by sajolida about 1 month ago

Pass-through the ?r= parameter to /donate/thanks and /donate/canceled (Will-fix: #15964)

Revision cec25af4
Added by intrigeri 11 days ago

Merge branch 'web/15964-pass-through-r' (Closes: #15964)

History

#1 Updated by sajolida about 1 month ago

  • 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 about 1 month ago

  • 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 29 days ago

  • Status changed from Confirmed to In Progress

#4 Updated by sajolida 29 days ago

  • File dom.png View added
  • File post.png View 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 28 days ago

  • QA Check changed from Dev Needed to Ready for QA

#6 Updated by intrigeri 11 days ago

  • % Done changed from 0 to 60

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

#7 Updated by intrigeri 11 days ago

  • 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 :)

Also available in: Atom PDF