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

only validate form data when changed #15389

Merged
merged 4 commits into from May 25, 2017
Merged

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented May 25, 2017

Addressing this comment

@Hamms Hamms requested a review from aoby May 25, 2017 19:28

def rollback!
restore_attributes
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Might this be simpler to derive DummyForm from ActiveRecord::Base?, and not have to implement these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, doing so means that it also wants to persist the data to the DB, which I don't care about for this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

But there's no harm right? Unit tests are run on the test environment (db) and also in a transaction so they won't actually leave anything behind. I think it would make the below test more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that I'd have to actually create a table for the dummy class, otherwise I get

Pd::FormTest
  test_pd_form_enforces_required_fields                          ERROR (0.01s)
ActiveRecord::StatementInvalid:         ActiveRecord::StatementInvalid: Mysql2::Error: Table 'dashboard_test.dummy_forms' doesn't exist: SHOW FULL FIELDS FROM `dummy_forms`
            test/models/concerns/pd/form_test.rb:43:in `block in <class:FormTest>'
            test/testing/setup_all_and_teardown_all.rb:22:in `run'

Let me poke around and see if there's a way I can inherit from ActiveRecord::Base and just tell it not to try to do that; I agree it would be much cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, I forgot each of these was a separate model, rather than the same with STI.

Maybe you can stub save and persisted? ? No need to spend a lot of time on this if it's complicated.

form.form_data = {firstField: "bar"}.to_json
assert_equal false, form.valid?
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get what this is testing. Why is foo valid and bar invalid? Also I'd like to explicitly see the case where an "invalid" form data is grandfathered in via save(validate: false), then shown to be valid unless modified.

nit: refute instead of assert_equal false,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, yeah, I can see how this is unclear.

That is essentially what I'm doing here; the only difference is that this dummy form I've constructed doesn't actually perform validation on save. Or, in other words, it always treats a save call as a save(validate: false).

Do you think adding comments explaining exactly what's going on here would be sufficiently clear? Or do I need to rethink this whole "DummyForm" idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

See above. I think it might be worth (and simpler) persisting to the (test) db. Otherwise yes please add comments.

@Hamms
Copy link
Contributor Author

Hamms commented May 25, 2017

Turns out it's pretty easy to just instantiate a temporary table to hold the ActiveRecord data! Much cleaner than what I was originally trying for.

PTAL

@Hamms Hamms merged commit e781c80 into staging May 25, 2017
@Hamms Hamms deleted the only-validate-form-data-if-changed branch May 25, 2017 23:03
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.

None yet

2 participants