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

[WIP] Adding FulfillmentOrder, FulfillmentRequest, CancellationRequest, AssignedFulfillmentOrder resources/filters #622

Closed
wants to merge 2 commits into from

Conversation

karmakaze
Copy link
Contributor

@karmakaze karmakaze commented Sep 13, 2019

Issue #211513 make shopify_api FO compatible

Add /orders/:order_id/fulfillment_orders resource

Reviewers please carefully check the move endpoint test in particular for form of request query/body and response.

@karmakaze karmakaze self-assigned this Sep 13, 2019
Copy link

@justinkrol justinkrol left a comment

Choose a reason for hiding this comment

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

I think this makes sense nested under the order resource, like you have it
Fixture mostly looks good; I think everything is coherent (like, proper request_status for an FO with an ApiFulfillmentService). The value of IDs doesn't really matter, right?

@karmakaze karmakaze force-pushed the fulfillment-order-resource branch from 3640f95 to d7a2108 Compare September 16, 2019 18:53
@karmakaze karmakaze changed the title Start adding FulfillmentOrder resource Add FulfillmentOrder resource Sep 16, 2019
@karmakaze karmakaze marked this pull request as ready for review September 16, 2019 18:54
@karmakaze karmakaze requested a review from a team as a code owner September 16, 2019 18:54
@karmakaze karmakaze force-pushed the fulfillment-order-resource branch from d7a2108 to 7a021a6 Compare September 16, 2019 19:21
@karmakaze karmakaze changed the title Add FulfillmentOrder resource [WIP] Adding FulfillmentOrder, FulfillmentRequest, CancellationRequest, AssignedFulfillmentOrder resources/filters Sep 16, 2019
@karmakaze karmakaze force-pushed the fulfillment-order-resource branch from 7a021a6 to 5366683 Compare September 19, 2019 20:21
@karmakaze
Copy link
Contributor Author

Adding fulfillment_request/cancellation_request actions to FulfillmentOrder resource.
Tried adding methods for filtering FOs by assigned_status/location_ids but it didn't like having its resource path altered from /orders/#/fulfillment_orders to /orders/#/assigned_fulfillment_orders.

Copy link
Contributor

@tanema tanema left a comment

Choose a reason for hiding this comment

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

I dont really have any knowledge about fulfillment however the code looks good

end
end

# context "#with_assigned_status" do

Choose a reason for hiding this comment

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

what is up with this commented out code?

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 was experimenting with how to organize assigned_fulfillment_orders either as a resource type matching the implemented endpoints or as methods on a fulfillment_orders resource from the client's perspective.

This can be deleted put into an assigned_fulfillment_orders_test.

@stevelounsbury
Copy link

Hey @karmakaze, are you still working on this one?

@karmakaze
Copy link
Contributor Author

@stevelounsbury Sorry I haven't had time to get back to Fulfillment Orders work, just finishing up the last of the Priority-1 Pickup In-store issues.

@karmakaze
Copy link
Contributor Author

karmakaze commented Jan 20, 2020

This work has been completed on PRs #633 #635 (#664) #637 (#665) #639 (#666).

@karmakaze karmakaze closed this Jan 20, 2020
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.

5 participants