Skip to content

Conversation

butchmarshall
Copy link
Contributor

All Submissions:

  • I've checked to ensure there aren't other open Pull Requests for the same update/change.
  • I've submitted a ticket for my issue if one did not already exist.
  • My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
  • I've used Github auto-closing keywords in the commit message or the description.
  • I've added/updated tests for this change.

Bug fixes and Changes to Core Features:

  • I've included an explanation of what the changes do and why I'd like you to include them.
  • I've provided test(s) that fails without the change.

Test Plan:

A unit test has been included with this pull request which reproduces the issue described.

Reviewer Checklist:

  • Maintains compliance with JSON:API
  • Adequate test coverage exists to prevent regressions

This fixes issue #1156 and provides a complete set of fixes for pull request #986.

Copy link
Member

@lgebhardt lgebhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Could you please rework the tests so this passes on rails 4.2?

@butchmarshall
Copy link
Contributor Author

Can do. It may take me a day or so but it'll definitely get done. Thanks!

@butchmarshall
Copy link
Contributor Author

butchmarshall commented Jul 10, 2018

Looks like it's failing because I added a key_type :uuid JSONAPI::Resource test (none had existed before).

Most likely related to rails/rails#24857

Workaround is to set self.primary_key = "id" on the resource.

I've pushed a fix (note my commenting out of the final assert_equal in my new test)

@lgebhardt lgebhardt merged commit 7c0c7ab into cerebris:master Jul 10, 2018
@lgebhardt
Copy link
Member

@butchmarshall Thanks for tracking that down and fixing!

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