{"action":"created","issue":{"url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/issues/1123","repository_url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth","labels_url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/issues/1123/labels{/name}","comments_url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/issues/1123/comments","events_url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/issues/1123/events","html_url":"https://github.com/lynndylanhurley/devise_token_auth/issues/1123","id":307801262,"number":1123,"title":"Confirming an already confirmed user -- still not quite working.","user":{"login":"Evan-M","id":395274,"avatar_url":"https://avatars2.githubusercontent.com/u/395274?v=4","gravatar_id":"","url":"https://api.github.com/users/Evan-M","html_url":"https://github.com/Evan-M","followers_url":"https://api.github.com/users/Evan-M/followers","following_url":"https://api.github.com/users/Evan-M/following{/other_user}","gists_url":"https://api.github.com/users/Evan-M/gists{/gist_id}","starred_url":"https://api.github.com/users/Evan-M/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Evan-M/subscriptions","organizations_url":"https://api.github.com/users/Evan-M/orgs","repos_url":"https://api.github.com/users/Evan-M/repos","events_url":"https://api.github.com/users/Evan-M/events{/privacy}","received_events_url":"https://api.github.com/users/Evan-M/received_events","type":"User","site_admin":false},"labels":[{"id":133878330,"url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/labels/request%20for%20comments","name":"request for comments","color":"5319e7","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-03-22T20:24:44Z","updated_at":"2018-05-24T13:02:56Z","closed_at":null,"author_association":"COLLABORATOR","body":"_**NOTE:** Much of this has been referenced before in several different Issues and PRs, which I will attempt to link to. If I've left any out, I apologize in advance._\r\n\r\n-------\r\n\r\nAlthough it appears that this was first noted in either #410 or #519, and eventually merged in #1001, the current behavior is still not great.\r\n\r\nAfter confirming a user's email address, I can continue to click the confirmation link (i.e. make GET requests to the `/auth/confirmations` endpoint, triggering the `devise_token_auth/confirmations#show` action).  It seems that the current behavior will generate a _new_ auth token for the user, and redirect to the successful confirmation URL (either from the params if present, or the `DeviseTokenAuth.default_confirm_success_url`).  \r\n\r\nAs pointed out in #1064, #1067, and #1075, when the _new_ token is created, it conditionally adds a expiration if the user has signed in (the `:sign_in_count` attribute), which creates a dependency on `:trackable`. And while #1064 + #1061 effectively uncouple the dependency, the result is that if `:trackable` is not included, then no expiration is set on the token! What-the-huh!!?\r\n\r\nThe confirmation process _should_ happen in a consistent manner, regardless of the presence of the `:trackable` module.  A review of the Devise internals shows that checking if a user has signed in (i.e. `@resource.has_attribute?(:sign_in_count) && @resource.sign_in_count > 0`) is **not an indicator of the user having been confirmed!**  Rather, it is the presence of a timestamp in the `:confirmed_at` attribute that indicates if a user has confirmed their account. (See: [`Devise::Models::Confirmable#confirm`] and [`Devise::Models::Confirmable#confirmed?`])\r\n\r\nFurthermore, the [`Devise::ConfirmationsController#show`] action simply and concisely checks for the presence of validation errors on the user resource, as it handles checking for already confirmed users at the model level. (The [`Devise::Models::Confirmable#confirm`] method sets errors on the user's `:email` attribute, if that user has already been confirmed).\r\n\r\nWe're already using the same [`Devise::Models::Confirmable::ClassMethods#confirm_by_token`] method in: https://github.com/lynndylanhurley/devise_token_auth/blob/38d27cfa80042c184c6f69e74c0789ca8ec0e407/app/controllers/devise_token_auth/confirmations_controller.rb#L3-L4\r\n\r\nTherefore, it _shouldn't_ really be necessary to check for specific attributes (or their values) on the user instance (e.g. `:sign_in_count`, `:confirmed_at`, etc...); just check for errors on the returned user instance like the [`Devise::ConfirmationsController#show`] action does.  The question is, _what_ does a failure response from the controller action look like?  It currently is just a 404 `Not Found`, but presumably it would call `DeviseTokenAuth::ApplicationController#render_error`, passing in the relevant validation errors where appropriate.\r\n\r\nLastly, it seems like the tests in the `confirmations_controller_test.rb` _not_ actually test most model level attributes; instead the tests should assert that various requests (each in known states) lead to the correct set of controller responses given those states.\r\n\r\nSpecifically, the tests for `:trackable` attributes should not exist at all here:\r\nhttps://github.com/lynndylanhurley/devise_token_auth/blob/38d27cfa80042c184c6f69e74c0789ca8ec0e407/test/controllers/devise_token_auth/confirmations_controller_test.rb#L55-L65\r\n\r\nAnd additional tests are needed to check the various possible validation errors on the user that _could_ happen when calling [`Devise::Models::Confirmable::ClassMethods#confirm_by_token`].\r\n\r\n--------\r\n\r\nI do intend on making a PR for all this, but I'm hoping to collect some feedback in the meantime. \r\n\r\n[`Devise::Models::Confirmable#confirm`]: https://github.com/plataformatec/devise/blob/75f9e76f65d2fd9892631efd0221ebe1921344b7/lib/devise/models/confirmable.rb#L76-L103\r\n[`Devise::Models::Confirmable#confirmed?`]: https://github.com/plataformatec/devise/blob/75f9e76f65d2fd9892631efd0221ebe1921344b7/lib/devise/models/confirmable.rb#L105-L108\r\n[`Devise::Models::Confirmable::ClassMethods#confirm_by_token`]: https://github.com/plataformatec/devise/blob/75f9e76f65d2fd9892631efd0221ebe1921344b7/lib/devise/models/confirmable.rb#L343-L360 \r\n[`Devise::ConfirmationsController#show`]: https://github.com/plataformatec/devise/blob/75f9e76f65d2fd9892631efd0221ebe1921344b7/app/controllers/devise/confirmations_controller.rb#L21-L32"},"comment":{"url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/issues/comments/391706467","html_url":"https://github.com/lynndylanhurley/devise_token_auth/issues/1123#issuecomment-391706467","issue_url":"https://api.github.com/repos/lynndylanhurley/devise_token_auth/issues/1123","id":391706467,"user":{"login":"dikond","id":9265682,"avatar_url":"https://avatars2.githubusercontent.com/u/9265682?v=4","gravatar_id":"","url":"https://api.github.com/users/dikond","html_url":"https://github.com/dikond","followers_url":"https://api.github.com/users/dikond/followers","following_url":"https://api.github.com/users/dikond/following{/other_user}","gists_url":"https://api.github.com/users/dikond/gists{/gist_id}","starred_url":"https://api.github.com/users/dikond/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dikond/subscriptions","organizations_url":"https://api.github.com/users/dikond/orgs","repos_url":"https://api.github.com/users/dikond/repos","events_url":"https://api.github.com/users/dikond/events{/privacy}","received_events_url":"https://api.github.com/users/dikond/received_events","type":"User","site_admin":false},"created_at":"2018-05-24T13:02:55Z","updated_at":"2018-05-24T13:02:55Z","author_association":"NONE","body":"To maintainers - thank you all for the gem and efforts to keep it up!\r\n@Evan-M Thanks for bringing it all together 👍 And a special thank you for discovering race condition which you described [here](https://github.com/lynndylanhurley/devise_token_auth/pull/1075#issuecomment-378779821)! It saved a lot of debugging time ❤️ \r\n\r\nI really like how simple devise controller looks using [`#errors`](https://github.com/plataformatec/devise/blob/master/app/controllers/devise/confirmations_controller.rb#L26) hope to see it one day here also :) \r\n\r\nI just came here to say that currently if the user was already confirmed we are returning an expired token. However, it makes more sense to return 422 error I believe. BTW, original devise action [does just that](https://github.com/plataformatec/devise/blob/master/app/controllers/devise/confirmations_controller.rb#L30)."}}
{"actor":{"display_login":"dikond"}}