Skip to content

Conversation

@jherey
Copy link
Contributor

@jherey jherey commented Apr 3, 2020

Summary

Add order number support on order service

Description

  • added function byOrderNumber
  • added orderNumber key to uri

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #1515 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1515   +/-   ##
=======================================
  Coverage   98.62%   98.62%           
=======================================
  Files         128      128           
  Lines        3275     3281    +6     
  Branches      754      757    +3     
=======================================
+ Hits         3230     3236    +6     
  Misses         41       41           
  Partials        4        4           
Impacted Files Coverage Δ
packages/api-request-builder/src/create-service.js 100.00% <100.00%> (ø)
packages/api-request-builder/src/default-params.js 100.00% <100.00%> (ø)
packages/api-request-builder/src/query-id.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48bf8b6...9f874c4. Read the comment docs.

Copy link
Contributor

@daern91 daern91 left a comment

Choose a reason for hiding this comment

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

Congrats on your first PR! 💯 🥇

It all looks good to me, and I just added a few nitpicks

expect(service.params.customerId).toBe('myCustomer')
})

test('should set the orderNumber param', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: It would be nice to move this block down below the test for customerId so they are all organized together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved it, take another look.


test('should throw if orderNumber is missing', () => {
expect(() => service.byOrderNumber()).toThrow(
/Required argument for `byOrderNumber` is missing or invalid/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/Required argument for `byOrderNumber` is missing or invalid/
/Required argument for `byOrderNumber` is missing/

another ocd nitpick from me 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, this is fixed now

@daern91 daern91 changed the title 1292 add byordernumber support on order service feat(request-builder): add orderNumber support on order service Apr 3, 2020
Copy link
Contributor

@danrleyt danrleyt left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jherey jherey force-pushed the 1292-add-byordernumber-support-on-order-service branch from 19ed3d8 to 9f874c4 Compare April 3, 2020 15:34
@jherey jherey merged commit 2a3fe05 into master Apr 3, 2020
@jherey jherey deleted the 1292-add-byordernumber-support-on-order-service branch April 3, 2020 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants