meteor-accounts-meld icon indicating copy to clipboard operation
meteor-accounts-meld copied to clipboard

Fix bug #21 via brettle:accounts-{add-service,accounts-multiple}

Open brettle opened this issue 9 years ago • 11 comments

Fix the "add new password account" case by just doing api.use('brettle:accounts-add-service').

Fix the "meld existing password account" case by copying logic from updateOrCreateUserFromExternalService() into a validateSwitch callback registered with brettle:accounts-multiple.

Add tests for both cases.

brettle avatar Sep 01 '15 23:09 brettle

I left the updateOrCreateUserFromExternalService monkey patch in place to handle the non-password case (as @splendido requested). FWIW, brettle:accounts-multiple could handle the non-password cases as well so that the monkey patch could be removed. Let me know if you'd be interested in a PR that would do that.

brettle avatar Sep 01 '15 23:09 brettle

Seems a good work!

So you would get rid also of the updateOrCreateUserFromExternalService function and use your package instead?

splendido avatar Sep 02 '15 06:09 splendido

Yes. Interested?

brettle avatar Sep 02 '15 15:09 brettle

To be honest I'd rather prefer to publish the perfect package under a non-private namespace, be it useraccounts or anything else.

Accepting a PR about this for this package would make it half mine and half not and I'm not sure I'll have the time to keep it up to date and grant its security.

Would you be willing to get access to a repo under meteor-useraccounts to develop useraccounts:multi-account or similar?

splendido avatar Sep 02 '15 15:09 splendido

Would useraccounts:multi-account be the same as (or similar to) brettle:accounts-multibrettle:accounts-multiple or would it be something else? I'd be happy to move any/all of the brettle:accounts-* suite (see brettle:accounts-deluxe for an overview) under meteor-useraccounts. Doing so would expand the scope of useraccounts (which is primarily UI oriented at the moment) to include lots of packages that don't have any UI component. If you're OK with that, I think it would be great.

brettle avatar Sep 02 '15 15:09 brettle

I saw you have a number of pacakges, but I had no time yet to check them out... :(

In case we're not talking about a huge number of code lines, wouldn't be it possible to have one single configurable package to rule them all? ;-)

splendido avatar Sep 02 '15 16:09 splendido

btw, the name useraccounts:multi-account was just a randomly typed one, any better naming will be accepted ;-)

splendido avatar Sep 02 '15 16:09 splendido

I prefer one package per feature so that app developers: 1) don't need to worry about configuration (just meteor add feature-package), 2) have less code to vet and 3) don't get upgraded as a result of changes to unrelated features. Also contributors have less code that they need to understand before contributing, and reuse is encouraged.

All that said, it's still possible to have one configurable package. It would just use the smaller packages and expose the necessary configuration options. This would require some minor changes to the existing smaller packages to support having them disabled by default, but that is doable.

brettle avatar Sep 02 '15 16:09 brettle

Should we start a discussion about this on the related trello card?

Perhaps also @zimme could be interested in participating ;-)

splendido avatar Sep 07 '15 19:09 splendido

I've joined in on the conversation on the trello card and I'll keep an eye on this PR too.

zimme avatar Sep 07 '15 19:09 zimme