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

Persist allow_password_change in the database #863

Conversation

MohamedBassem
Copy link
Contributor

@MohamedBassem MohamedBassem commented Apr 7, 2017

Fixes #481
@resource.allow_password_change is not persisted across requests which makes it impossible to reset a user's password after a password reset when config.check_current_password_before_update is set to :password.
This pull request:
1- Modifies the generators to add the new column.
2- Adds a new test to cover this case.
3- Removes the old attribute accessors.

@bernica
Copy link

bernica commented Apr 27, 2017

I think you should add the new attribute to the except list at token_validation_response (User concern) so it doesn't get serialized.

@bernica
Copy link

bernica commented Sep 8, 2017

@MohamedBassem @lynndylanhurley Any news on this?

@zachfeldman
Copy link
Contributor

Makes sense to me and no merge conflicts. Didn't review the code but will try to come back to this soon has anyone else reviewed the code here? @bernica perhaps you can commit the change you recommended?

@lynndylanhurley
Copy link
Owner

I'm cleaning up the test suite and the original test around this feature is now failing. Looking at this now and I'm not sure how it was ever passing.

lynndylanhurley added a commit that referenced this pull request Oct 14, 2017
@lynndylanhurley lynndylanhurley merged commit db8d694 into lynndylanhurley:master Oct 15, 2017
@misham
Copy link

misham commented Jan 2, 2018

I know this has already been merged in and I'm coming late to the party but how are existing users supposed to leverage this now? Is the expectation to regenerate the User migration or .... ?

Some kind of docs on this would be amazing. Happy to write something up, but given the current state of the code, the only way forward I see is a manually generated migration.

If this is the expected path forward, let me know and I can create a wiki page.

@lynndylanhurley
Copy link
Owner

@misham - that's a good question. should we include a new migration for the fix? how is this situation usually handled by other gems?

@misham
Copy link

misham commented Jan 2, 2018

The "easy button" option would be a separate migration that is created for existing installs for the default case. That is, User model exists with the default columns.

Bonus points would be to use the configuration in the existing app to detect what the model is/are and create migration(s) for everything that's needed.

If the above is not feasible, then an upgrade section in the README would be the next best thing.

One caveat to the above, I solved this by implementing a version of the solution proposed here: #481 (comment)

So I had to back my changes out before I anything else. Not sure how that plays into this though.

@zachfeldman
Copy link
Contributor

@misham I'd be down to see a pull request for either that migration and/or an upgrade to the README. Thanks for your suggestions.

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.

5 participants