-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fulfillment feature #6480
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
Fulfillment feature #6480
Conversation
setFulfillmentType, methodAdditionalData, cartVersion Signed-off-by: Sujith <mail.sujithvn@gmail.com>
supportedFFTypes allowedValues to dynamic array in FFT-base Signed-off-by: Sujith <mail.sujithvn@gmail.com>
supportedFFTypes allowedValues to dynamic array in FFT-base Signed-off-by: Sujith <mail.sujithvn@gmail.com>
supportedFFTypes allowedValues to FFT-base and methodAdditionalData Signed-off-by: Sujith <mail.sujithvn@gmail.com>
new endpoint validateOrder & refactored placeOrder Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
First commit of the base api-plugin-fulfillment Signed-off-by: Sujith <mail.sujithvn@gmail.com>
First commit of the api-plugin-fulfillment-type-shipping Signed-off-by: Sujith <mail.sujithvn@gmail.com>
First commit for api-plugin-fulfillment-type-pickup Signed-off-by: Sujith <mail.sujithvn@gmail.com>
First commit of the api-plugin-fulfillment-pickup-store Signed-off-by: Sujith <mail.sujithvn@gmail.com>
First commit of the api-plugin-fulfillment-method-shipping-ups Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
First commit - api-plugin-fulfillment-shipping-flat-rate Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
|
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly changes around style. Will test more tomorrow
apps/reaction/tests/integration/api/mutations/checkout/checkoutTestsCommon.js
Outdated
Show resolved
Hide resolved
...action/tests/integration/api/mutations/shippingRates/createFlatRateFulfillmentMethod.test.js
Show resolved
Hide resolved
@@ -0,0 +1,70 @@ | |||
# api-plugin-fulfillment-method-shipping-ups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uncomfortable with calling this shipping-ups
since it doesn't actually implement UPS. I would rather it be called something like "dynamic-rate"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, if we are planning to expand this to a more functional (and less dummy) plugin, then we could try out ups as the first option and retain the name.
If that is not the plan, we can change the name as you have suggested.
...api-plugin-fulfillment-method-shipping-ups/src/getFulfillmentMethodsWithQuotesShippingUPS.js
Outdated
Show resolved
Hide resolved
packages/api-plugin-fulfillment-method-shipping-ups/src/util/sampleTest.test.js
Show resolved
Hide resolved
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the PR partially. It's quite a daunting task to review within days something that's been developed for months. Eventually will get to it :)
if (!cart.cartVersion) { | ||
supportedFulfillmentTypes = ["shipping"]; // we use 'shipping' as default fulfillment type for old v1 carts (prior to fulfillment types) | ||
} else { | ||
throw new ReactionError("not-found", "Product does not have any supported FulfillmentTypes"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the supportedFulfillmentType on a product and on a catalog is not mandatory. This causes the supportedFulfillmentTypes
to be an empty array by default (packages/api-plugin-products/src/mutations/createProduct.js)
We always will hit this error when adding to cart products that didn't had explicit supportedFulfillmentTypes
set. We either have to make the CreateProductInput.supportedFulfillmentTypes
required, or provide a default list of types. Otherwise, the system can get into an invalid state.
The invariant of the product catalog is that it should always have at least one supported ff type. If it doesn't, it cannot be fulfilled by any method and therefore it shouldn't be visible in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a separate PR (#6543) which helps the admin to set base ff-types and these ff-types would get added to all new products along with (any) user provided ff-types.
packages/api-plugin-carts/src/resolvers/Mutation/setFulfillmentTypeForItems.js
Show resolved
Hide resolved
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
packages/api-plugin-orders/src/xforms/xformOrderFulfillmentGroupSelectedOption.js
Show resolved
Hide resolved
@@ -14,26 +14,30 @@ | |||
"accounts": "@reactioncommerce/api-plugin-accounts", | |||
"authentication": "@reactioncommerce/api-plugin-authentication", | |||
"authorization": "@reactioncommerce/api-plugin-authorization-simple", | |||
"products": "@reactioncommerce/api-plugin-products", | |||
"catalogs": "@reactioncommerce/api-plugin-catalogs", | |||
"products": "../../packages/api-plugin-products/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use @reactioncommerce/...
for all plugins including new ones.
|
||
const pickupRecord = await Fulfillment.findOne({ fulfillmentType: "pickup", shopId }); | ||
if (!pickupRecord) { | ||
const groupInfo = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this code duplicated on other fulfillment types. could this be done in a declarative way via register function? I saw the same pattern for fulfillment methods too.
cc: @zenweasel
rates.push({ | ||
carrier, | ||
handlingPrice: updatedMethod.handling, | ||
method: updatedMethod, | ||
rate: updatedMethod.rate, | ||
shippingPrice: updatedMethod.rate + updatedMethod.handling, | ||
shopId: doc.shopId | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be done in a declarative way too?
each method will provide a function that returns new rates.
the root plugin will collect all of rates to return so we won't have previousQueryResults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are collecting rates from multiple methods under same ff-type. We also give each method more than one chance to return the rates (since they could be fetched from external systems). So if initial fetch attempt fails, they are added into retrialTargets
This PR is replaced by PR#6480 |
Signed-off-by: Sujith mail.sujithvn@gmail.com
Resolves #6471
Impact: breaking
Type: feature
Issue
In the current system, the only way for order fulfillment is via shipping which is hardcoded into the system. We need to introduce the flexibility for users to add their own fulfillment types via plugins.
Solution
We are creating a base fulfillment plugin which would enable other fulfillment types (like shipping, pickup, digital) to be introduced via plugins. Each of the newly introduced fulfillment type plugin would need to have the specific fulfillment methods also to be added as separate plugins. Example, fulfillment type 'pickup' could have fulfillment methods like 'store pickup' and 'curb-side pickup'.
Breaking changes
Since this is the new way of handling fulfillment, the existing shipment plugins would not work along with this. User would need to migrate the data (migration script will be developed) and remove the shipment plugins (api-plugin-shipments and api-plugin-shipments-flat-rate).
Testing
To perform review and testing of the related code changes, we are adding details related to this feature implementation which spans across multiple plugins (each in its own folders in the packages folder in monorepo).
At a high level the impacted plugins are:
Existing plugin changes
api-plugin-products
api-plugin-catalogs
api-plugin-carts
api-plugin-orders
Plugins to be removed
api-plugin-shipments
api-plugin-shipments-flat-rate
New Plugins to be added
api-plugin-fulfillment
api-plugin-fulfillment-type-shipping
api-plugin-fulfillment-method-shipping-flat-rate
api-plugin-fulfillment-method-shipping-ups
api-plugin-fulfillment-type-pickup
api-plugin-fulfillment-method-pickup-store
Changes in detail:
CARTS
CATALOGS
PRODUCTS
ORDERS
(above 2 changes are spread across multiple files)
ORDERS (specific to placeOrder refactor and introduction of validateOrder end-point)
API-PLUGIN-FULFILLMENT
This is the base plugin, basic features listed below.
Mutations
Queries
Others
API-PLUGIN-FULFILLMENT-TYPE-SHIPPING
API-PLUGIN-FULFILLMENT-TYPE-PICKUP
API-PLUGIN-FULFILLMENT-METHOD-SHIPPING-FLAT-RATE
API-PLUGIN-FULFILLMENT-METHOD-SHIPPING-UPS
API-PLUGIN-FULFILLMENT-METHOD-PICKUP-STORE
Test cases
With the above changes, we should be able to test the following cases:
Pending items
The below pending items will be introduced via separate PR/tickets