+
Skip to content

New place order refactor validate order #6616

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

Merged
merged 15 commits into from
May 18, 2023

Conversation

sujithvn
Copy link
Contributor

@sujithvn sujithvn commented Nov 3, 2022

Signed-off-by: Sujith mail.sujithvn@gmail.com

Resolves #6471
Impact: breaking
Type: feature

This PR replaces #6583

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.

There is no way to verify if the order data in the UI is ready/complete and free of errors.

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'.

Along with the changes, we are creating a new API query end-point validateOrder which provides the option for frontend to validate the correctness and completeness of the order before actually placing the order.

Also, we are refactoring the placeOrder mutation and cutting down the functional layers

This PR is 5th entry (new-placeOrder refactor validateOrder)

PR Details in order

feat/fulfillment-types
 | 
 | __ 00 fulfillment base #6610 
 |        | __ new-fulfillment-type-shipping      #6614
 |        | __ new-fulfillment-type-pickup.    #6613 
 |__ new-fulfillment-impacted-plugins. [carts, products, orders, catalogs] #6615 
 |__ new-placeOrder refactor validateOrder  <--- This PR. #6616

Existing PRs based on the #6480 (to be updated to point to #6610)

We shall change the base branch of the 3 existing PRs (i18n #6545, dataMigration #6544, setDefaultFFtype #6543) to point to 00 fulfillment base

Details (specific to placeOrder refactor and introduction of validateOrder end-point)

  • Moved out most of the functionalities from placeOrder to prepareOrder.
  • Flattened out the hierarchical function calls by 1-2 levels and placed all the called functions in folder orderValidators inside util. Most of the functions are copied over from existing code base.
  • prepareOrder builds up the order object and performs the validation steps in parallel. Validation results are accumulated in an array for most of the errors except the initial 2-3 errors where we return with the error.
  • Both placeOrder and validateOrder calls prepareOrder with respective flags.
  • Few steps specifically are related to payments auth and are skipped in case validateOrder is calling prepareOrder.
  • In case of validateOrder call, most of the function calls are placed in a try-catch block to collect the errors. In case of placeOrder call, functions are directly executed.
  • Final order object is validated against the schema (in case of prepare order, it is validated against a custom schema).
  • prepareOrder returns Order & token to placeOrder, and returns success flag and validationResults to validateOrder.
  • placeOrder will finally insert the order returned by prepareOrder into the DB and completes the placing of order.

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

All this is new code and at this stage this is not integrated to reaction, meaning the plugins.json is not updated to include this plugin. Hence proper testing can be carried out only after merging in the remaining PRs.

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>
@changeset-bot
Copy link

changeset-bot bot commented Nov 3, 2022

⚠️ No Changeset found

Latest commit: 5b36b12

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@sujithvn sujithvn changed the base branch from trunk to 00-fulfillment-base November 8, 2022 16:47
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

Aside from the few inline suggestions, prepareOrder is a good example of a fn where returning errors rather than throwing them would be a lot cleaner. You wouldn't need to try/catch and formatErrors everywhere. I'm not sure whether you want to tackle that level of refactoring across all those functions now, but it would be good to do eventually.

}

group.shipmentMethod = {
const output = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this will no longer be mutating the group object, maybe it should be in a new function with a different name, such as getShipmentMethodForGroup?

* @param {String} flag - flag which define if the call is from placeOrder or validateOrder
* @returns {Promise<Object>} output - order, token and validation results
*/
export default async function prepareOrder(context, input, flag) {
Copy link
Member

Choose a reason for hiding this comment

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

This function is quite long with many duplications. This should be cleaned up.

@sujithvn sujithvn removed the request for review from tedraykov January 9, 2023 04:14
sujithvn added 10 commits March 10, 2023 22:42
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
…ateOrder

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
…ctor-validateOrder

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
…or-validateOrder

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
…efactor-validateOrder

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
…ction into 00-fulfillment-base

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
…ateOrder

Signed-off-by: Sujith <mail.sujithvn@gmail.com>
@sujithvn sujithvn merged commit 344bbf6 into 00-fulfillment-base May 18, 2023
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.

As a user, I should be able to checkout with multiple fulfillment types in a single order using the new set of plugins
3 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载