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.

Sign up for GitHub

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

Jump to bottom

Find and inflate unpopulated resources returned to api-client [P4-2337] #981

Merged
merged 2 commits into from
Nov 17, 2020
Merged

Find and inflate unpopulated resources returned to api-client [P4-2337] #981

merged 2 commits into from
Nov 17, 2020

Conversation

solidgoldpig
Copy link
Contributor

Proposed changes

What changed

  • Find and inflate unpopulated resources returned to api-client

Modifications to common/lib/api-client/middleware/request.js and common/lib/api-client/middleware/process-response.js allow for minimal resource stubs to be added to the included section.

common/lib/find-unpopulated-resources.js allows for the detection of unpopulated resources in any data structure passed to it

common/lib/populate-resources.js allows for the inflation of any unpopulated resources in any data structure passed to it

  • Enable move#getById to ask for population of resources

Why did it change

Some backend endpoints do not allow for the retrieval of resources using includes.

Devour sets any resource referenced as a relationship but not present in the included section to null.

Issue tracking

Screenshots

n/a

Checklists

Testing

Automated testing

  • Added unit tests to cover changes
  • Added end-to-end tests to cover any journey changes

Manual testing

Has been tested in the following browsers:

  • Chrome
  • Firefox
  • Edge
  • Internet Explorer

Environment variables

  • No environment variables were added or changed

Other considerations

@teneightfive teneightfive temporarily deployed to booksecuremove-pr-981 November 13, 2020 08:46 Inactive
merlinc
merlinc previously requested changes Nov 13, 2020
View reviewed changes
common/lib/populate-resources.test.js Show resolved Hide resolved
common/lib/populate-resources.js Show resolved Hide resolved
common/lib/find-unpopulated-resources.js Outdated Show resolved Hide resolved
common/lib/find-unpopulated-resources.js Outdated Show resolved Hide resolved
common/lib/api-client/middleware/request.js Outdated Show resolved Hide resolved
common/lib/api-client/middleware/process-response.js Outdated Show resolved Hide resolved
@solidgoldpig solidgoldpig force-pushed the handle-unpopulated-resources-p4-2337 branch from e47c31f to 222bb23 Compare November 13, 2020 13:33
@solidgoldpig solidgoldpig temporarily deployed to booksecuremove-pr-981 November 13, 2020 13:33 Inactive
@solidgoldpig solidgoldpig force-pushed the handle-unpopulated-resources-p4-2337 branch from 222bb23 to 2193a66 Compare November 13, 2020 15:12
@solidgoldpig solidgoldpig temporarily deployed to booksecuremove-pr-981 November 13, 2020 15:12 Inactive
@solidgoldpig solidgoldpig force-pushed the handle-unpopulated-resources-p4-2337 branch from 2193a66 to 275bc2c Compare November 13, 2020 15:14
@solidgoldpig solidgoldpig temporarily deployed to booksecuremove-pr-981 November 13, 2020 15:14 Inactive
teneightfive
teneightfive reviewed Nov 15, 2020
View reviewed changes
Copy link
Contributor

@teneightfive teneightfive left a comment

Choose a reason for hiding this comment

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

Might be good to jump on a call Monday morning just to try and understand the purpose of this change a little better.

It feels like this is overriding some, possibly quite specific, functionality within the JSON:API client library and I'm a little worried about whether this might have knock-on effects elsewhere within the application.

@merlinc merlinc dismissed their stale review November 16, 2020 18:13

Tests have been simplified

merlinc
merlinc approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@merlinc merlinc left a comment

Choose a reason for hiding this comment

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

LGTM

teneightfive
teneightfive approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@teneightfive teneightfive left a comment

Choose a reason for hiding this comment

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

As raised during our calls, I have concerns about this change as it changes the behaviour with the way the API client handles deserialisation and could lead to some unexpected side-effects/uneasy debugging.

Can we please add a GH issue to our backlog to get this removed once the API correctly supports includes.

@solidgoldpig solidgoldpig mentioned this pull request Nov 17, 2020
Open
@solidgoldpig
chore: Find and inflate unpopulated resources returned to api-client
8954ee0 
Some backend endpoints do not allow for the retrieval of resources using includes.

Devour sets any resource referenced as a relationship but not present in the
included section to null.

Modifications to `common/lib/api-client/middleware/request.js` and
`common/lib/api-client/middleware/process-response.js` allow for minimal resource stubs
to be added to the included section.

`common/lib/find-unpopulated-resources.js` allows for the detection of unpopulated resources
in any data structure passed to it

`common/lib/populate-resources.js` allows for the inflation of any unpopulated resources
in any data structure passed to it
@solidgoldpig
chore: Enable move#getById to ask for population of resources
d033aaf
@solidgoldpig solidgoldpig force-pushed the handle-unpopulated-resources-p4-2337 branch from 275bc2c to d033aaf Compare November 17, 2020 10:53
@solidgoldpig solidgoldpig temporarily deployed to booksecuremove-pr-981 November 17, 2020 10:53 Inactive
@codeclimate
Copy link

codeclimate bot commented Nov 17, 2020

Code Climate has analyzed commit d033aaf and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 97.9% (0.1% change).

View more on Code Climate.

@solidgoldpig solidgoldpig merged commit 941aadc into master Nov 17, 2020
@solidgoldpig solidgoldpig deleted the handle-unpopulated-resources-p4-2337 branch November 17, 2020 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlinc merlinc merlinc approved these changes

@teneightfive teneightfive teneightfive approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@solidgoldpig @merlinc @teneightfive