Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Integrate initial package claiming PRs #4305

Merged
merged 4 commits into from
May 9, 2017
Merged

Conversation

chadwhitacre
Copy link
Contributor

@chadwhitacre chadwhitacre commented Jan 24, 2017

This is an integration branch/PR for the initial implementation of package claiming. Part of #4427.

Specs

See #4297 (comment).

Mocks

From #4297 (comment) (click for full-size):

PRs (in merge order)

Numbers are workflow steps.

&c.

Todo

  • think through merging/takeover—anything special here? like, start verification workflow, then merge accounts, then finish verification.—looks like we already do something9b7d147
  • what about linking an existing project to an existing npm package? … sounds like an admin thing; bump for now, handle through email support
  • make sure finishing a verification terminates competing verifications (otherwise get_emails can include an email it shouldn't)
  • think through PayPal requirement—are we accounting for that somewhere here that I'm not seeing?
  • consider notifying other package maintainers when someone claims a package … bumped: Notify other maintainers on package claim #4425
  • indicate to a participant when they have a claim open … bumped: Show open claims to participant #4426
  • check it on a phone … bumped: Fix on a phone #4432
  • account for deleted packages: Harmonize package deletion and claiming #4448

Landing

  • change base back to master
  • merge and deploy!

@nobodxbodon
Copy link
Contributor

Could you share the plan of this project?

Besides, I guess you haven't started paying for gomockingbird? Shall we apply to balsamiq earlier than later?

@chadwhitacre
Copy link
Contributor Author

Could you share the plan of this project?

Not sure what you mean. I updated the todo in the ticket description. Not enough?

Besides, I guess you haven't started paying for gomockingbird?

Nope.

Shall we apply to balsamiq earlier than later?

Not a priority for me. a) Balsamiq appears to be desktop-only, which is of limited utility for us since we can't share links, b) I think we have what we need from Mockingbird for now, and c) I think we can hack Mockingbird by deleting cookies/localstorage.

@nobodxbodon
Copy link
Contributor

Not sure what you mean. I updated the todo in the ticket description. Not enough?

Maybe just me, but I don't find a easy way to tell which part a dev (me perhaps) can pick up without worrying about stepping on your toe.

a) Balsamiq appears to be desktop-only, which is of limited utility for us since we can't share links,

they offer choices in application including web version:
screen shot 2017-01-24 at 2 50 44 pm

c) I think we can hack Mockingbird by deleting cookies/localstorage.

Seriously?

@chadwhitacre
Copy link
Contributor Author

Maybe just me, but I don't find a easy way to tell which part a dev (me perhaps) can pick up without worrying about stepping on your toe.

I think we should get you unblocked and focusing on gratipay/finances#35. I'm happy to do the bulk of the work here but I will need you @mattbk @JessaWitzel etc. for review. Let's work with who we have.

they offer choices in application including web version:

Go for it!

Seriously?

😊

@chadwhitacre
Copy link
Contributor Author

@nobodxbodon at #4297 (comment):

When there are multiple contact emails for one package, indicating there are more than one devs, do we send notification emails out to all of them in case the package is claimed by one of the email?

Good idea!

@chadwhitacre
Copy link
Contributor Author

We'll also need to account for the case where an email is attached to a different Gratipay account. They won't be able to use it in that case.

@chadwhitacre
Copy link
Contributor Author

Todo reordered to reflect merge order.

@chadwhitacre
Copy link
Contributor Author

Picking up from #4297 (comment):

I was wondering the same thing. Which email/person do we allow to claim the package.

We pull email addresses from the maintainers and authors in package.json on npm.

@nobodxbodon
Copy link
Contributor

In case you missed #4297 (comment), shall we notify all the contacts after step 8 of spec, and states so to the claimer in the email something like "we'll notify all the other coauthors of the package in this email list after you confirm claiming"?

@kaguillera
Copy link
Contributor

kaguillera commented Jan 26, 2017

  1. Is any one of the Co-authors of a package allowed to claim the packages?
  2. What if the co-author is a new-comer claims the package before the real starter of the package gets a chance to.

That was really what I was trying to ask earlier (#4297 (comment))

@chadwhitacre
Copy link
Contributor Author

What's up with the failures here? I seem to recall expecting this ...

@chadwhitacre
Copy link
Contributor Author

Still looking for the perfect process here. I keep switching the todo order. Currently working stepwise through the workflow.

I've got the confirmation flow (3-8) half roughed-in. I think at this point I'm going to go back over and clean up and merge #4155 so that we have a Package object from which to hang API, and then pick up again with #4309. At some point we also could/should rebase this on master to pick up recent changes there.

@chadwhitacre
Copy link
Contributor Author

Is any one of the Co-authors of a package allowed to claim the packages?

No (under):

The thing that seems the most canonical representation from what I've seen around probably is whatever the owners are of the actual npm package vs. what's in the package.json.

Sent a follow-up in private email to clarify:

Looking at the registry, I see three options:

  1. _npmUser—scoped to version
  2. maintainers—scoped to package
  3. author—scoped to package

My hunch based on our convo is that the _npmUser for the latest version is the best one to use for linking (though the _ suggests that this is a private API, and a spot check shows it is missing for some versions, maybe very old ones?). Does that sound right to you?

@chadwhitacre
Copy link
Contributor Author

Alright, based on that and considerations at #4309 (comment) I've added another prereq:

update schema for a single (nullable) email & sync from the latest version's _npmUser

@chadwhitacre chadwhitacre changed the title Implement claim/unclaim/reclaim packages ✈️ Implement claim/unclaim/reclaim packages Feb 8, 2017
@chadwhitacre
Copy link
Contributor Author

Okay! Shelving work on #4309. I had thought of two PRs right after that one (implementing .send_confirmation_email), and then confirmation link post-back). But now it seems I should dial back to working on npm syncing prereqs.

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Feb 8, 2017

Scratch that! 💃

Taking a look at a few packages, it is definitely the maintainers field that I had in mind, as the corresponds to the users who have the rights to publish the package, which is the most likely to align to the folks who would manage any funds, imo

Aligns with doco:

Add a new user as a maintainer of a package. This user is enabled to modify metadata, publish new versions, and add other owners.

Note that there is only one level of access. Either you can modify a package, or you can't. Future versions may contain more fine-grained access levels, but that is not implemented at this time.

https://docs.npmjs.com/cli/owner

npm also sets a top-level "maintainers" field with your npm user info.

https://docs.npmjs.com/files/package.json#people-fields-author-contributors

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Feb 8, 2017

But we do want to drop author.

@chadwhitacre
Copy link
Contributor Author

/me adds that to prereqs ...

@chadwhitacre
Copy link
Contributor Author

Back into #4309!

This was referenced Feb 9, 2017
@chadwhitacre chadwhitacre mentioned this pull request Feb 20, 2017
@chadwhitacre
Copy link
Contributor Author

Squashed and rebased onto #4343, was 5bab99a.

@chadwhitacre chadwhitacre mentioned this pull request Apr 28, 2017
@chadwhitacre
Copy link
Contributor Author

Awesome. All PRs are in!

I guess we should spend some effort on syncing npm before we deploy this, though. :-/

@chadwhitacre
Copy link
Contributor Author

The snapshot of npm we have in the Gratipay database is six months old. We need to update that and also build in a mechanism for constantly updating it.

@chadwhitacre chadwhitacre mentioned this pull request May 3, 2017
8 tasks
@chadwhitacre
Copy link
Contributor Author

Rebased and squashed, was 70b391e.

@chadwhitacre
Copy link
Contributor Author

Okay, friends! I believe we are ready to land this sucker!

@chadwhitacre
Copy link
Contributor Author

@clone1018 and I are going to merge and deploy this.

Includes: #4424 #4428 4435
- Move upsert & delete onto Package
- Make verification results easier to debug
- Smooth out a slight very wrinkle in return value for paypal_updated
- Implement and test desired behavior
@chadwhitacre
Copy link
Contributor Author

Rebased, was 8a2e258.

@chadwhitacre
Copy link
Contributor Author

Travis is running super-slow.

@chadwhitacre
Copy link
Contributor Author

Finally green! Ready!?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants