Skip to content

Add a kwarg to override relationships rendered in jsonapi errors #6

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Startouf
Copy link

@Startouf Startouf commented Jul 25, 2017

This especially allows to render jsonapi errors without a jsonapi payload (such as GET requests) by passing relationships: {}

Features

  • Pass relationships: false to send empty second argument
  • relationships: Argument is optional
  • Non breaking changes (default remains to send controller deserialized relationships)

…jsonapi errors

This especially allows to render jsonapi errors without a jsonapi payload (such as GET requests) by passing `relationships: {}`
@Startouf Startouf changed the title Add a keyword parameter to override relationships to be rendered for … Add a kwarg to override relationships rendered in jsonapi errors Jul 25, 2017
@richmolj
Copy link
Contributor

Thanks for this PR!

I think my preference would be relationships: false instead of relationships: {}. Would that make sense to you?

@Startouf
Copy link
Author

Startouf commented Jul 25, 2017

I actually meant to commit nil but forgot to copy my local changes to the branch request.
You mean by passing false we should not pass any relationship argument to the call to the constructor ?

My suggestion in this case

def render_errors_for(record, relationships: nil)
      relationships = if relationships == false
        {}
      else
        relationships || deserialized_params.relationships
      end

      validation = Serializers::Validation.new(record, relationships)
end

So we need to explicitely pass relationships: false to skip relationships. Otherwise we can pass manually a relationships argument to skip using deserialized_params.relationships
(And tis is non breaking : without the relationships param we get the default deserialized_params.relationships)

@richmolj
Copy link
Contributor

Having thought about this a bit more, I think your original commit was probably the way to go. I'm looking for

def render_errors_for(record, relationships: {})
# ... code ...
  relationships || deserialized_params.relationships # at some point

If we get that + a spec I'll merge 👍

# Defaults to the deserialized data.relationships of Json:api Payload
# @param record [ ActiveModel ] Object that implements ActiveModel
def render_errors_for(record, relationships: {})
relationships || deserialized_params.relationships
Copy link
Contributor

Choose a reason for hiding this comment

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

this would never hit the right side of || since this defaults to {}. Maybe line 6 should be nil, but you can still pass a custom hash?

Copy link
Author

Choose a reason for hiding this comment

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

Hey thanks and sorry about that, I'm still working on it and the test, didn't mean to push that commit ^^"
Regarding the test, since it's quite a big integration test I need some more time to work on it

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.

2 participants