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

Avoid patch on RelationshipView deleting relationship instance when constraint would allow null #499

Merged
merged 2 commits into from
Oct 25, 2018

Conversation

Alig1493
Copy link
Contributor

@Alig1493 Alig1493 commented Oct 22, 2018

Fixes #242

Description of the Change

Added test to check for relationship view relationship types and patch accordingly and updated relationship functionality for related fields

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • changelog entry added to CHANGELOG.md
  • author name in AUTHORS

@Alig1493
Copy link
Contributor Author

Alig1493 commented Oct 22, 2018

@sliverc An attempt to resolve the issue #242.

@Alig1493 Alig1493 changed the title Added test to check for relationship view relationship types and patc… Patch Relationship View Oct 22, 2018
@Alig1493 Alig1493 changed the title Patch Relationship View Relationship View Patch Oct 22, 2018
@Alig1493 Alig1493 force-pushed the relationship_view_patch branch 2 times, most recently from 187057f to e3c66b0 Compare October 22, 2018 14:01
Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Nice work! This looks good to me.

See inline comments and also a CHANGELOG entry needs to be added.

rest_framework_json_api/views.py Outdated Show resolved Hide resolved
rest_framework_json_api/views.py Outdated Show resolved Hide resolved
example/tests/test_views.py Outdated Show resolved Hide resolved
example/tests/test_views.py Outdated Show resolved Hide resolved
@Alig1493
Copy link
Contributor Author

@sliverc I applied the suggested changes.

Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Two linting issues (see inline comments) which concern this PR.

There are other linting issues but that's because deps are not pinned. Needs to be fixed in another PR (will see that I get to it)

'id': str(self.second_comment.id)
}
],
'links': {'self': 'http://testserver/authors/{}/relationships/comment_set'.format(self.author.id)}
Copy link
Member

Choose a reason for hiding this comment

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

./example/tests/test_views.py:246:101: E501 line too long (110 > 100 characters)

'id': str(comment.id)
}
],
'links': {'self': 'http://testserver/authors/{}/relationships/comment_set'.format(self.author.id)}
Copy link
Member

Choose a reason for hiding this comment

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

/example/tests/test_views.py:259:101: E501 line too long (110 > 100 characters)

@sliverc
Copy link
Member

sliverc commented Oct 25, 2018

master is now fixed after merging #501 so a rebase to master will fix the other linting issues.

@Alig1493
Copy link
Contributor Author

@sliverc I have rebased and pushed.

@sliverc sliverc changed the title Relationship View Patch Avoid patch on RelationshipView deleting relationship instance when constraint would allow null Oct 25, 2018
…h accordingly and updated relationship functionality for related fields
@sliverc sliverc force-pushed the relationship_view_patch branch 2 times, most recently from 368abde to ea6cbfc Compare October 25, 2018 14:45
@sliverc sliverc merged commit b3eed32 into django-json-api:master Oct 25, 2018
@sliverc
Copy link
Member

sliverc commented Oct 25, 2018

@Alig1493 Thanks. Added missing changelog entry and merged.

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