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

Adding in unlocks controller and specs. This should resolve #927. #971

Merged
merged 4 commits into from
Oct 11, 2017

Conversation

brycesenz
Copy link
Contributor

This should resolve the core issue of #927. However, it has these known shortcomings (and perhaps more that I haven't identified):

  1. The redirect after an unlock is just the root path (i.e. '/'), and it is set in a controller method. I'm not as familiar with how the library does configuration and default handling, but this would make more sense as a configurable option.
  2. The messages for the Unlocks controller only have English translations in the locales

@zachfeldman
Copy link
Contributor

Thanks so much @brycesenz !

Any reason some of the tests are commented out? Otherwise looks pretty good to me, especially for a first cut.

@brycesenz
Copy link
Contributor Author

@zachfeldman - oh, haha, probably just human error. Like I said, I'm awful with Minitest - I couldn't figure out the command to test just one spec in isolation (as opposed to the whole file), so I took to commenting things out once they worked. Please let me know the rest of your thoughts once you've had a chance to review.

Also, as should be clear from the code, I added an API endpoint to let a user trigger an unlock email to be sent to his/her account (a create method in the controller). The specs cover that as well, but I do want to call it out since it's functionality beyond the scope just of this issue.

@zachfeldman
Copy link
Contributor

zachfeldman commented Oct 9, 2017

No problem, mind uncommenting that so we can run Travis against it?

Yeah that sounds like a good thing to have! Thanks for pointing it out.

@zachfeldman
Copy link
Contributor

All checks have passed. Going to approve this - if one more person does, I will merge.

@zachfeldman zachfeldman self-requested a review October 9, 2017 20:36
@MaicolBen
Copy link
Collaborator

Nice work there, I feel you about Minitest, however, you did great testing!

But I don't like some copy/pasted code from SessionsController. In fact you didn't take into account a recent change in the providers' query: #964

Can you in some way refactor the first part of the create method to a concern or to the DeviseTokenAuth::ApplicationController ? Let us know if you can't.

@brycesenz
Copy link
Contributor Author

@MaicolBen - Haha, yeah, I ripped a lot of stuff from the PasswordsController actually. The two are very similar! That provider change has actually not made its way into the PasswordsController yet I see - good reason to abstract it! Is that the only part of the create action that you were talking about refactoring? It seems like a fair bit of the library could use some refactoring (no offense; that's the nature of open source sometimes), but I don't want to have the scope of this PR bleed too far.

@brycesenz
Copy link
Contributor Author

Ok, submitted a refactor with a new module. Some parts of the code still aren't really clear to me - for example, this block appears in theApplicationController, but if I try to move it to a separate module, specs start failing. And minitest unfortunately gives me so many warnings that it's hard to track down what's actually going wrong.

    def resource_class(m=nil)
      if m
        mapping = Devise.mappings[m]
      else
        mapping = Devise.mappings[resource_name] || Devise.mappings.values.first
      end

      mapping.to
    end

@MaicolBen
Copy link
Collaborator

You didn't give me time to tell you to not do it if it were too much effort! But you did great work! I will review it. I'm thinking in cleaning it more in the future.

@lynndylanhurley
Copy link
Owner

It seems like a fair bit of the library could use some refactoring

Agreed. The code climate score for this project is really embarrassing x_x

@brycesenz
Copy link
Contributor Author

@lynndylanhurley - But the test coverage is solid! I mean for real - I would not have had the confidence to change anything if I didn't have a test suite to run against to make sure I didn't break anything. Y'all have done an awesome job keeping the test suite in order!


@resource = resource_class.where(q, @email).first
@email = get_case_insensitive_field_from_resource_params(:email)
@resource = find_resource(:email, @email)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be :uid instead?

@resource = lockable_users(:unlocked_user)
end

describe 'not email should return 401' do
Copy link
Collaborator

@MaicolBen MaicolBen Oct 10, 2017

Choose a reason for hiding this comment

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

Is this message correct? I would put request unlock without email

@brycesenz
Copy link
Contributor Author

@MaicolBen - Both of those points are valid. I have incorporated those changes into the PR.

@zachfeldman
Copy link
Contributor

Looks good to me! Love the ResourceFinder abstraction. Merging

@zachfeldman zachfeldman merged commit 6423aad into lynndylanhurley:master Oct 11, 2017
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.

4 participants