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
Pass options
into server-side log-in lifecycle hooks
#13085
base: devel
Are you sure you want to change the base?
Conversation
api.export([ | ||
'pollUntil', 'try_all_permutations', | ||
'SeededRandom', 'clickElement', 'blurElement', | ||
'focusElement', 'simulateEvent', 'getStyleProperty', 'canonicalizeHtml', | ||
'renderToDiv', 'clickIt', | ||
'withCallbackLogger', 'testAsyncMulti', | ||
'simplePoll', 'runAndThrowIfNeeded', | ||
'makeTestConnection', 'DomUtils']); | ||
'blurElement', | ||
'canonicalizeHtml', | ||
'clickElement', | ||
'clickIt', | ||
'DomUtils', | ||
'focusElement', | ||
'getStyleProperty', | ||
'makeTestConnection', | ||
'MockFunction', | ||
'pollUntil', | ||
'renderToDiv', | ||
'runAndThrowIfNeeded', | ||
'SeededRandom', | ||
'simplePoll', | ||
'simulateEvent', | ||
'testAsyncMulti', | ||
'try_all_permutations', | ||
'withCallbackLogger', | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely understand if you want me to revert this and just add the single thing to make this diff better. I just ... really wanted to make this cleaner for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this.
Aside: running the test suite locally to verify it's all passing, it finished like this: "All tests pass!" but "Passed 1598 of 1599" 🤔 EDIT: Oh hmm, maybe this is significant. This is what happened in CI:
I'm not seeing what that 1 failure is, though... hopefully not related to my change. |
* Provides server-side parity with the work started in meteor#11913 * Attaches any `options` provided in the result of the log-in handler to the `attempt` * Adds a small suite of tests covering client-side and server-side places where `options` is expected to be available
b238d8b
to
4677819
Compare
Hey @nazrhyn, do you need this for Meteor 2? It might take a while before we have another version 2 release. But we could include this in the next 3.0-rc |
@denihs Thanks for the follow up! We have a local fork that includes this change in our current version, so there's no urgency on our side. We just wanted to contribute this back in case the Meteor team liked it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the only thing is that I would probably target this on the 3.x branch.
Would you like me to rebase it or is that something y'all will do? I don't mind, just need the exact branch name. |
Best to rebase on |
Details
Currently, any
options
provided by the log-in handler that services a log-in attempt are not passed to the server-sideAccounts.onLogin
and.onLoginFailure
lifecycle hooks as part of theattempt
. We use these lifecycle hooks for various integrations and application features, and it would be nice to be able to transport custom information between those two locations. Currently, we're doing something very unpleasant to bridge that gap.This change modifies
Accounts._attemptLogin
, following the pattern forerror
anduser
, attachingoptions
to theattempt
object being built there. This is really the only material change, but I've also done some more work.Related to this change is the following PR where a co-worker of mine made a similar change for the client-side
userCallback
ofAccounts.callLoginMethod
. My change brings parity on the server-side, making sureoptions
are passed everywhere they might be useful.As a gesture of good will 😇, I have also written a small suite of tests that covers:
Accounts.validateLoginAttempt
hookAccounts.onLogin
hookAccounts.onLoginFailure
hook(These tests essentially cover my co-worker's PR changes from before.)
validateResult
callbackuserCallback
callbackPotentially Contentious Things
MockFunction
class and added it totest-helpers
. I considered it to be an expedient in writing these tests, but if it's too new or out of line with what you want in your test suite, please let me know.accounts_tests_setup.js
, but please let me know if I did that wrong.Accounts.registerLoginHandler
function with a test-only handler. I wanted to write tests that were focused in scope, and so that's how I went about it, rather than relying on existing handlers that might've been added in other packages.