Add bulk claiming at /on/npm/

[chadwhitacre]

Picking up from #4416.

Todo

• move package API to a mixin: Refactor ahead of bulk npm #4495
• insert into emails during make_participant: Refactor ahead of bulk npm #4495
• add get_packages_for_claiming
• modify get_packages_for_claiming to sort/group as desired
• link to the email settings page
• set the avatar as the npm logo
• disable apply when there are no claimable packages
• indicate when a package is already claimed, differentiating claimed-by-self and -by-other
• fix test suite
• wire up to emails/modify.json, send to address displayed
• verify that we test what happens when someone tries to claim an already claimed package
• reuse open review ticket where possible—bumping to Reuse open review tickets where possible #4505
• tie into review process better

[chadwhitacre]

Working up a ttw test.

[chadwhitacre]

I'm working on this commit that actually adds the email address during make_participant (instead of just blindly inserting it). It's a simple change (fc36e0c) but has follow-on effects for the rest of the test suite. I'm down to one more failure, and it's weird. It's in test_teams.py, and it's an interaction between several tests, with the failure manifesting in test_error_message_for_slug_collision. It's a failure to overwrite a VCR file—but! it only shows up if five other tests are present. Of the six I've got with it right now (a3872a5) if any of the other five are gone the test passes. If all other six are present then it fails. 🐨

[chadwhitacre]

Heck with it. Let's just rerecord the VCR fixture. 😉

[chadwhitacre]

Moved off this PR into #4495.

[chadwhitacre]

[chadwhitacre]

Rebased on master, was 4587ba8.

[chadwhitacre]

Bringing over some local notes:

think through what happens when I put someone else's email in there                                           
    they get a link that's like "Claim your packages on Gratipay!"                                            
    they think, "Um, okay, why not?"                                                                          
        they think that we sent it to them                                                                    
    they click                                                                                                
    they are now sharing an account with an attacker                                                          
                                                                                                              
    I think we need to confirm again on landing                                                               
        "You are linking to thus and such account"                                                            
                                                                                                              
    this is a general problem with email verification, not specific here                                      
                                                                                                              
    At the very least we need to have the message in the email be "Sound familiar?"                           
        Try to communicate that they supposedly initiated the action

[chadwhitacre]

[chadwhitacre]

[chadwhitacre]

Ready for review, @rohitpaulk! Commits are a bit sprawling, pushing the limit on max lines delta. Lmk if I should factor out a PR for ease.

[chadwhitacre]

Rebased, was c95af1e.

[mattbk]

It all seems to work for me, but (as a new user), what do I do once I'm here? What's the next step?

• I got emails relating to "connecting these packages to my account." I don't see anything about the review process.
  • Should https://github.com/gratipay/gratipay.com/blob/master/emails/project-review.spt emails be sent out (ideally concatenated to one message)?
  • Or should the regular post-apply message be displayed?
    

I feel like we're not closing the loop toward people knowing what they need to be able to receive money. Maybe this is less of a problem with developers? (i.e., do they read more than regular people?)

[mattbk]

I like the automatic PayPal route creation if a person doesn't have the PayPal route set up yet. 👍

[chadwhitacre]

#4515 factored out, was a42f002.

[chadwhitacre]

Also factored out #4516 and #4517. More manageable now, @rohitpaulk? :-)

[chadwhitacre]

Rebased, was 6fc28cc.

[rohitpaulk]

Might be worth adding a note saying that we searched using the email addresses a, b, and c - and to update on npm if needed (just like the page for a single package)

[rohitpaulk]

Oh wait, that is present below. Ignore.

[rohitpaulk]

I think the naming here could be better. These values are only being pulled so that we can show the x packages out of y are on Gratipay - might be better to keep them in a single dict so that a confusion doesn't arise between npackages, Npackages and packages_for_claiming.

How about - npm_stats = website.db.one('''select () as total_packages, () as claimed_packages''', return_as=dict), followed by n=npm_stats.total_packages, N=npm_stats.claimed_packages (in the template)?

[chadwhitacre]

Done in abfef42!

[rohitpaulk]

Hmm, we always have an address - why not show it in the link?

[rohitpaulk]

i.e. Why do we have to say Check your inbox when we have an address available

[rohitpaulk]

Is it because we send emails to multiple addresses?

[rohitpaulk]

I only see two emails being sent in start_email_verification - one is a verification notice to the primary email, the other is the actual verification link to the address being verified.

[chadwhitacre]

Because it's a content spoofing vector. I thought we had a HackerOne ticket about this but it's not jumping out at me on http://inside.gratipay.com/appendices/disclosures.

[chadwhitacre]

This is confusing 😕

D'oh! 😞

verification.spt is used both to verify emails and to verify linking packages to emails.

Yeah, it seemed to me that it was best to have one code path for both, since we want linking packages to emails to also verify the email. That is, we don't want to make the user go through two verification flows (one for the address itself, a second for the packages) when from their point of view one should be sufficient. Am I thinking about that right?

The pattern of if conditions suggests that there isn't much in common either

Are you referring to the if action == 'start-verification' block? That's the extra bit for the package claiming case, where we load the packages in question. The flows rejoin just after, in the call to start_email_verification, which is where the bulk of the common wiring lives.

I think it is best to separate these out.
i.e. use separate functions (and templates) for verifying an email vs verifying linking an email to a package

I agree it's pretty dense (especially when we get to finish_email_verification), but I think it's necessary to support the simpler, single-step user experience during package claiming. Do you see a way to refactor while preserving the single-step flow?

[rohitpaulk]

Yeah, it seemed to me that it was best to have one code path for both, since we want linking packages to emails to also verify the email. That is, we don't want to make the user go through two verification flows (one for the address itself, a second for the packages) when from their point of view one should be sufficient. Am I thinking about that right?

I see how this makes sense. In that case, do we absolutely need to re-verify emails when claiming packages? If a user has a verified email, can't we just allow them to claim the package directly?

[chadwhitacre]

Decided in slack to reticket dropping the re-verification.

[rohitpaulk]

^ Discussed on Slack.

[chadwhitacre]

Reticketed as #4520.

[chadwhitacre]

Rebased, was abfef42.