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

issue 4356 adds is_navigational_format? check to after_sign_up_path_for #4833

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

iorme1
Copy link
Contributor

@iorme1 iorme1 commented Apr 4, 2018

Added is_navigational_format? check to after_sign_up_path_for, as the issue #4356 requested. Tests are passing.

@@ -112,7 +112,7 @@ def sign_up(resource_name, resource)
# The path used after sign up. You need to overwrite this method
# in your own RegistrationsController.
def after_sign_up_path_for(resource)
after_sign_in_path_for(resource)
after_sign_in_path_for(resource) if is_navigational_format?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we cover this change with tests?

Copy link
Contributor Author

@iorme1 iorme1 Apr 8, 2018

Choose a reason for hiding this comment

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

I looked up all the current methods in Devise that use 'is_navigational_format?' checks and I couldn't seem to find anything in those tests that specifically incorporate a test for that navigational check. In my previous pull requests for Devise, I wrote my tests based off similar structured methods and their corresponding tests. However, since I cannot find how the other methods using 'is_navigational_format?' are testing it, I am not sure how to
incorporate additional tests for the method I added it to. Based on the issue conversation, i was under the impression that the method I changed used to have a 'is_navigational_format?' check and that it somehow got deleted by accident from a git reset. So I simply added it back to the method.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we don't have tests for any of these so I think it's ok for now.

@tegon tegon merged commit acc45c5 into heartcombo:master Aug 2, 2018
@tegon
Copy link
Member

tegon commented Aug 2, 2018

Thank you @iorme1! 🎉

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

Successfully merging this pull request may close these issues.

3 participants