Skip to content

Conversation

@mengqing
Copy link

redirect_url should be set to DeviseTokenAuth.default_password_reset_url if not present in the options

@resource = create(:user, :confirmed)
@redirect_url = 'http://ng-token-auth.dev'

DeviseTokenAuth.default_password_reset_url = @redirect_url
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redirect_url isn't null, why this change in the spec ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was to test "Using default_password_reset_url", however previously the redirect url was passed as a param, and that's why the spec passed, and thus the change in the spec to show that the test is still valid without the url being passed to the controller

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and how the spec in line 61 passes? we have a validation for the missing redirect url in the controller

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mengqing ☝️

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaicolBen My bad, it looks like on

DeviseTokenAuth.default_password_reset_url
, DeviseTokenAuth.default_password_reset_url is already used as a backup for @redirect_url, hence the spec in line 61 passes.

The issue I wanted to fix in this PR is really for resource.send_reset_password_instructions where the method would still work if no redirect_url is passed as an option, eg: calling it directly. Since there isn't a test to cover send_reset_password_instructions, do I need to create a new test for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can, add a spec for this case in the model

… be set to default url if not present in opts
@mengqing mengqing force-pushed the fix-send-reset-password-instructions-redirect-url branch from 552a753 to 2ada9f3 Compare August 17, 2019 09:15
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.

2 participants