Skip to content

Validate .update method #289

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Validate .update method #289

wants to merge 1 commit into from

Conversation

@kkirsche
Copy link
Contributor

@kkirsche kkirsche commented Nov 17, 2014

Follow rails convention by returning false unless the item is valid.

@karmi
Copy link
Contributor

karmi commented Apr 11, 2015

This makes sense! Apologies for the ridiculous delay, you should have pinged me!

If you still have time & energy for the patch, can you please update it:

  1. There's been a :validate option introduced in 1fd6c0b, can we play with it?
  2. Can you please add a unit test for this?

@karmi karmi self-assigned this Apr 11, 2015
@kkirsche
Validate .update method
8901130 
Follow rails convention by returning false unless the item is valid.
@kkirsche
Copy link
Contributor Author

kkirsche commented Apr 13, 2015

Updated with :validate option and unit tests. Please let me know if you have any questions, comments or concerns.

karmi
karmi reviewed Apr 23, 2015
View reviewed changes
elasticsearch-persistence/test/unit/model_store_test.rb
Copy link
Contributor

@karmi karmi Apr 23, 2015

Choose a reason for hiding this comment

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

Have you run the test? subject.expects(:persisted?).returns(true) ... :)

Copy link
Contributor Author

@kkirsche kkirsche Apr 23, 2015

Choose a reason for hiding this comment

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

Test suite of this project has never worked for me sadly so I haven't been able to outside of Travis CI

@karmi karmi closed this in 50fee81 Apr 23, 2015
karmi added a commit that referenced this pull request Apr 23, 2015
@karmi
[STORE] Fixed tests for the updates to the update method for Persis…
b660d6a 
…tence::Model

This fixes 50fee81

Related: #289
@karmi
Copy link
Contributor

karmi commented Apr 23, 2015

Fixed the tests in b660d6a and merged.

@dgmora
Copy link

dgmora commented Jan 17, 2017

Unless I'm miss understanding something, this is not fixing the important cases.

If you have stored a valid model, and then update with an invalid attribute, valid? will be true, as the model has yet not been changed, and there won't be validation

@dgmora dgmora mentioned this pull request Jan 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@karmi karmi

Labels

feature waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kkirsche @karmi @dgmora