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

onLogout should provide connection information #7397

Closed
gsonnenf opened this Issue Jul 13, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@gsonnenf

gsonnenf commented Jul 13, 2016

The onLogout hook was added with a comment that it was analogous to the onLogin hook:

Adds Accounts.onLogout() a hook directly analogous to Accounts.onLogin()

https://github.com/meteor/meteor/blob/devel/History.md#v1331

I've tested it and it doesn't seem to provide session context analogous to the onLogin hooks.

The login hook code is as follows - 'packages/accounts-base/accounts_server.js' ->
line: 165

Ap._successfulLogin = function (connection, attempt) {
  this._onLoginHook.each(function (callback) {
    callback(cloneAttemptWithConnection(connection, attempt));
    return true;
  });
};

line 355:
this._successfulLogin(methodInvocation.connection, attempt);

The logout hook code is as follows - 'packages/accounts-base/accounts_server.js' ->
line 179:

Ap._successfulLogout = function () {
  this._onLogoutHook.each(function (callback) {
    callback(); <---- no context set
    return true;
  });
};

line 535:

 methods.logout = function () {
    var token = accounts._getLoginToken(this.connection.id);
    accounts._setLoginToken(this.userId, this.connection, null);
    if (token && this.userId)
      accounts.destroyToken(this.userId, token);
    this.setUserId(null);
    accounts._successfulLogout(); // <-------------no arguments passed
  };

It would be useful to pass a connection context and perhaps also return the the userId of the person who logged out as an argument (it is currently cleared before the log out is called).

@gsonnenf gsonnenf changed the title from onLogout doesn't provide session information as expected to onLogout doesn't provide connection information as expected Jul 13, 2016

@abernix abernix changed the title from onLogout doesn't provide connection information as expected to onLogout should provide connection information Jul 13, 2016

@abernix

This comment has been minimized.

Member

abernix commented Jul 13, 2016

Thanks for this feature request. I think this is reasonable, and this will likely be a candidate for pull-requests-encouraged, however I'll cite the feature request guidelines for the sake of completeness and because there are a lot of other feature requests:

You'll have the best chance of getting a change into core if you can build consensus in the community for it. Start by creating a well specified feature request as a Github issue.

Help drive discussion and advocate for your feature on the Github ticket (and perhaps the forums). The higher the demand for the feature and the greater the clarity of it's specification will determine the likelihood of a core contributor prioritizing your feature by flagging it with the pull-requests-encouraged label.

Specifically, try to work up a consensus (perhaps on the forums) of what fields would be most beneficial, etc. Shouldn't be too much to figure out here, but +1's accepted!

Related to #6889 #7187

@gsonnenf

This comment has been minimized.

gsonnenf commented Jul 14, 2016

Sounds good, thanks for the advice. I was under the impression onLogout was suppose to have a connection context, being analogous to onLogin, according to the patch notes and #7187, so I assumed there was already consensus and that the initial pull omitted this on accident.

@Ragsboss

This comment has been minimized.

Ragsboss commented Jul 14, 2016

+1

@Ragsboss

This comment has been minimized.

Ragsboss commented Jul 16, 2016

I can volunteer to fix this. Please let me know if anyone else is working on it. Otherwise I'll start working on this early next week (7/18)..

@abernix

This comment has been minimized.

Member

abernix commented Jul 18, 2016

This issue is up for grabs! Well written pull-requests will be accepted if they meet the requirements. Please read Contributing, include new tests whenever possible, ask if you have implementation questions and reference this issue number when opening a pull request. 😄

You can run tests locally for the accounts-base package by running this command from a meteor checkout.

./meteor test-packages accounts-base
@ghost

This comment has been minimized.

ghost commented Jul 18, 2016

+1

@Ragsboss

This comment has been minimized.

Ragsboss commented Jul 19, 2016

Please review my pull request for this bug at #7433

@laosb

This comment has been minimized.

Collaborator

laosb commented Aug 8, 2016

Close since the PR was merged.

@laosb laosb closed this Aug 8, 2016

@interfaith

This comment has been minimized.

interfaith commented Sep 24, 2016

after logging in with twitter a hook is needed to cleanup before logging out.
does that exist now ?

@abernix

This comment has been minimized.

Member

abernix commented Oct 4, 2016

@interfaith It does exist as of 1.4 but apparently the docs for that feature did not get published when 1.4 came out. I'll see about getting that pushed to 1.4 docs, but meanwhile, you can view the commit here: meteor/docs@9072697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment