Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brettle
Copy link

@brettle brettle commented Sep 1, 2015

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.

…ple}

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
Copy link
Author

brettle commented Sep 1, 2015

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.

@splendido
Copy link
Owner

Seems a good work!

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

@brettle
Copy link
Author

brettle commented Sep 2, 2015

Yes. Interested?

@splendido
Copy link
Owner

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?

@brettle
Copy link
Author

brettle commented Sep 2, 2015

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.

@splendido
Copy link
Owner

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
Copy link
Owner

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

@brettle
Copy link
Author

brettle commented Sep 2, 2015

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.

@splendido
Copy link
Owner

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

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

@zimme
Copy link

zimme commented Sep 7, 2015

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

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

Successfully merging this pull request may close these issues.

3 participants