Skip to content

dont include linked resources already present in the response#13

Merged
davobutt merged 3 commits intomasterfrom
FP-1042
Sep 20, 2017
Merged

dont include linked resources already present in the response#13
davobutt merged 3 commits intomasterfrom
FP-1042

Conversation

@kevinhodges
Copy link
Copy Markdown
Contributor

@kevinhodges kevinhodges commented Sep 5, 2017

What does this PR do? (please provide any background)

https://hxshortbreaks.atlassian.net/browse/FP-1042
Some The Works endpoints now create their own linked resources (see https://github.com/holidayextras/the-works/pull/590 which will consume this work once merged) removing the need for plugin-jsonapi to request them. Plugin-jsonapi now handles this by checking for the presence of linked includes before adding them onto the request array.

This is a performance/optimisation piece.

What tests does this PR have?

One additional test to cover the additional usecase of the getSubResources function

How can this be tested?

On it's own, not so much. Pull down https://github.com/holidayextras/the-works/pull/590 and run a GET /packageRates request with an include of ticketRates,roomRates,hotelProducts. You will then see plugin-jsonapi only request the hotelProducts as the packageRates endpoint will return the ticketRates and roomRates resources itself.

Any tech debt?

None

Screenshots / Screencast

What gif best describes how you feel about this work?

By approving a review you are confirming you have...

  • Witnessed the work behaving as expected (this could be on the author's machine or screencast).
  • Checked for coding anti-patterns.
  • Checked for appropriate test coverage.
  • Checked all the tests are passing.

Comment thread lib/utilities.js Outdated
// We already have these resources
return
}
if (resource.links && resource.links[include]) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not really in your change, but don't think you can get here and resource.links not be truthy.

Given that you could I suppose go with

         if (resource.links[include] && !(stateObject.linked && stateObject.linked[include])) {
          utilities._addSubResourceRequest(stateObject.subResourceRequests, resource.links[include])
        } 

But not sure that adds much to readability.

Comment thread test/lib/utilitiesTest.js
return expect(utilities.getSubResources(stateObject)).to.eventually.deep.equal(expected)
})
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Free Gift ;-)
I wrote this test to satisfy myself...

    it('should not attach any already linked sub resources, but still add unlinked ones', function () {
      var stateObject = {
        resources: [
          {
            id: 'ID',
            links: {
              LINK1: {
                ids: ['LINK_ID1'],
                type: 'LINK1'
              },
              LINK2: {
                ids: ['LINK_ID2'],
                type: 'LINK2'
              }
            }
          }
        ],
        includes: ['LINK1', 'LINK2'],
        linked: {
          LINK1: []
        }
      }

      var expected = _.cloneDeep(stateObject)
      expected.subResourceRequests = {
        LINK2: {
          ids: ['LINK_ID2']
        }
      }
      return expect(utilities.getSubResources(stateObject)).to.eventually.deep.equal(expected)
    })

@davobutt davobutt merged commit 74ec809 into master Sep 20, 2017
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