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

Add before_validation hooks to resources #153

Merged
merged 3 commits into from Jun 20, 2019

Conversation

A-Boudi
Copy link

@A-Boudi A-Boudi commented May 26, 2019

Solves #122

This adds a before_validation hook to be fired just before the before_commit hook.

@richmolj
Copy link
Contributor

Thanks @A-Boudi - I want to take a closer look soon, but just glancing looks like a great PR. Just want you to know it's on my radar 👍

end

it "does not run before_commit callbacks" do
expect_any_instance_of(Graphiti::Util::ValidationResponse)
Copy link
Contributor

Choose a reason for hiding this comment

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

You did a great job with the general setup, but I think this is the issue that would hold up the PR. We wanted something to fire before validation. As you can see and line 126 of resource_proxy.rb, this is happening after validation. IOW, we want to_not receive - and probably a Rails integration spec that addresses the original issue.

I don't think you're too far off though! Hope we get this in 👏 !

@A-Boudi
Copy link
Author

A-Boudi commented May 31, 2019

Hi, @richmolj, I'm not sure if I did understand the issue correctly.

Here is what I originally understood:
During the persistance of a resource, graphiti works like this:

  • Process resource's parents (mainly belongs_to relationships)
  • Persist the resource with the foreing keys retrieved above
  • Process resource's children (has_many and many_to_many relationships). Persist the children using the resource key.

If a resource has validation on its children, it can't be saved because the children are not accessible when the resource is being saved.

To fix this issue, the validation on children are scoped via a :whatever context. And a second round of validation is done using :whatever after that all the resource's parents and children are persisted and accessible.

Now, if we want something to fire before validation. Would this mean that this new callback should be fired before before_save callback?

@A-Boudi
Copy link
Author

A-Boudi commented Jun 12, 2019

Hi @richmolj, what do you think?

@richmolj
Copy link
Contributor

@A-Boudi thanks for the reminder and sorry for the delay. I think you have correct understanding of the issue, but the word validation is tripping things up. Graphiti "validation" is actually just checking model, instances for #errors - we expect the actual validation to be done with ActiveModel and performed during save.

So, this is maybe this is better named as before_validation_processing or after_graph_persisted or something like that.

Still, it should fire before Graphiti::Util::ValidationResponse code fires. Otherwise we'd be performing the validation, but nothing would happen in response (no rolling back the transaction or rendering errors correctly).

@A-Boudi A-Boudi force-pushed the 122-add-before-validation-hook branch 2 times, most recently from 9c9c769 to 83fbfcb Compare June 17, 2019 15:49
@A-Boudi A-Boudi force-pushed the 122-add-before-validation-hook branch from 83fbfcb to 42269e9 Compare June 18, 2019 13:21
@A-Boudi
Copy link
Author

A-Boudi commented Jun 18, 2019

Hi @richmolj,

Like asked, I moved this hook to fire before graphiti validation. I also changed the name to after_graph_persist.

For the tests, I have kept the test introduced in the first commit. And I added new integration tests that addresses the original issue.

@richmolj
Copy link
Contributor

Really, REALLY great job @A-Boudi 💯 🥇 ! Thanks so much for sticking with it - I really appreciate the integration tests in particular ❤️ If you could please add an entry to the CHANGELOG, I'd be happy to merge!

@A-Boudi
Copy link
Author

A-Boudi commented Jun 19, 2019

@richmolj CHANGELOG updated!

@richmolj richmolj merged commit 5f65c15 into graphiti-api:master Jun 20, 2019
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