-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Better support for shipping method providers (single method, multiple prices / options) #2649
Comments
A somewhat related issue is error handling in shipping calculators. If an api call fails, it either has to throw which fails the entire getEligibleShippingMethods query, or return a fallback price; neither of which are great. |
Hi @michaelbromley , I'm diving into this feature request and looking forward to coding it up. Given its importance for my e-commerce platform, I wanted to double-check before starting my experiments. Are there any specific areas or functions I should be particularly careful not to modify? |
@michaelbromley i've added the code here github it would be awesome to get some insights from you before moving forward. |
Hi @brunoslalmeida I just took a look through your fork. I think the overall direction looks good. I have some feedback on the API changes: export const TEST_SHIPPING_METHOD = gql`
query TestShippingMethod($input: TestShippingMethodInput!) {
testShippingMethod(input: $input) {
eligible
quote {
price
priceWithTax
metadata
}
list {
eligible
quote {
price
priceWithTax
metadata
}
}
}
}
`;
I would prefer this: query TestShippingMethod($input: TestShippingMethodInput!) {
testShippingMethod(input: $input) {
eligible
quote {
price
priceWithTax
metadata
}
quotes {
price
priceWithTax
metadata
}
}
} and we can deprecate the |
Yes, it is redundant it was already in plans to remove it. I was also struggling to find a name, quotes seams like a good name. I will fix it and continue. |
Hey @michaelbromley just finished your feedback changes (i was struggling with time) and will get the pace back this week. Thanks for your support. |
Hi @michaelbromley, I have a question. As far as I'm studying, the entry point for shipping selection at shop-api is the mutation My only current option is to create a new optional argument that receives an array with the same size as the first one containing IDs to identify the inner ID. If no information is sent by the front-end, then the first item of each method should be selected. To achieve this, an optional ID needs to be added to For admin-api the entry-point is What are your thoughts on this? Am i missing something important ? |
Hi, regarding the
Once we make changes to the API, we can update the storefront implementations as needed. |
I'm also looking for this functionality and can contribute to the implementation if there's a need, just let me know where I could be helpful. I agree with the backward-compatible change suggested above. Don't know if there is an established pattern for deprecation but it would be good to discourage the use of the old signature after release so as to enable removing it at some point. |
@brunoslalmeida a small comment on your changes: Instead of having Same goes for |
Hey @kbanman we are facing a possible breaking change at other places. I've discuss with @michaelbromley at discord and will bring it here. The problem is that for on shippingMethod with multiple results there must be a new field (this is the db breaking) to select witch item from the results is being selected. |
This is the discussion thread. https://discord.com/channels/1100672177260478564/1218990088403288074 |
@kbanman I do also think that retrocompatibility here might not be necessary. As soon as we handle the breaking problem with @michaelbromley I will test this change. |
So one way we could handle this in a non-breaking way would be to put it behind a feature flag like shippingOptions: {
multipleQuotesPerShippingMethod: true
} and depending on that setting, we can dynamically define the field in the DB and extend the GraphQL API. In general I'm not a big fan of adding feature flags like this, because they complicate the code base in multiple places. But I'm not sure if there's a better solution without breaking things. An alternative is to leave this for a future major release. |
The ideal solution would be to leave this for future major. I am currently planning to create a custom base plugin that allows developers to create their own solution with its supplier as a temporary solution while this is not solved via major. This custom base plugin will have a function base param so the developer can create it's own calls to the supplier and the plugin will handle the multiple shipping function using custom fields to add the selected method to the order. |
@michaelbromley I am moving back to this issue and would like your help with the feature flag and entity. I am a bit confused how to validade the feature flag at the entity file in order to just allow the new field creation if the parameter is true, can you give-me some light ? |
I am also thinking that it might be necessary to add validation if the calculator is returning an array and the multipleQuotesPerShippingMethod is false and notify user that this is problem. @michaelbromley Is this ok for you? |
Since entity properties (columns) are defined statically as class members with decorators, we'd need to take a different approach to add a property at run-time based on a flag. Luckily we do have a mechanism for this: EntityMetadataModifier. So the idea would be to define a modifier function that adds the desired column to the entity during bootstrap. This happens here for user-defined modifiers. You could create one and run it directly after that in the case that the feature flag is set on. Since this is adding a bit of complexity, try to think of ways to keep all flag-related code organized in a way that is clear and will be easy to refactor when we come to the next major and want to make it the default.
Yes that sounds reasonable. |
Ok, thanks I will take a look and do homework. |
@michaelbromley help clarify some doubts.
Do you have any other input ? |
The migration message "Your database schema does not match your current configuration..." is not actually part of the bootstrap process. It is shown in a standard installation because by default we run the
Yes - the metadata modifier needs to be executed at that point.
This is a unique identifier for the given quote from a single ShippingMethod right? I think
This is out-of-scope for this specific issue, but the code makes me think we need a proper framework for implementing feature flags in a cleaner way. Right now we would have feature-flag code scattered around the code base. For one or two feature flags this is tolerable (if ugly) but the tendency is always towards more chaos :D so maybe it is a good time now to think about what the right way would be to handle this. Imagine something like this: we define a abstract class FeatureFlag {
name: string;
abstract determineEnabledStatus(config: RuntimeVendureConfig): boolean;
protected _isEnabled: boolean;
get isEnabled() {
return this._isEnabled;
}
constructor(config: RuntimeVendureConfig) {
this._isEnabled = this.determineEnabledStatus(config);
}
} and then we define the flag like this: // packages/core/src/feature-flags/multiple-shipping-quotes.ts
export class MultipleShippingQuotesFeatureFlag extends FeatureFlag {
name: 'Multiple shipping quotes',
determineEnabledStatus(config) => config.shippingOptions.multipleQuotesPerShippingMethod === true,
// other logic/methods can be defined as methods
//on the feature flag class to better encapsulate things
addShippingMethodCode(config: VendureConfig) {
if (!config.entityOptions.metadataModifiers) {
config.entityOptions.metadataModifiers = [];
}
const addShippingMethodCode: EntityMetadataModifier = metadata => {
const instance = new ShippingLine();
Column({ type: 'text', nullable: true })(instance, 'code');
};
config.entityOptions.metadataModifiers.push(addShippingMethodCode);
}
} At some point in the bootstrap process we instantiate all the feature flags:
and use it like this: // bootstrap.ts
if (FEATURE_FLAG.multipleShipping.isEnabled) {
multipleShippingQuotesFeatureFlag.addShippingMethodCode(config);
} Note: I don't expect you to implement this. I just wanted to jot down my idea as it came to me. For now you can continue development and it should be easy to refactor to use whatever feature flag system we implement in the future. |
Added suggestion from issue vendure-ecommerce#2649
Cross linking #3085 here, as feature flags have been discussed in this thread. |
Is your feature request related to a problem? Please describe.
There are many shipping providers (e.g. shipstation, shippo, stamps.com, etc) which provide APIs which return an undetermined number of shipping methods with prices; currently in vendure a single Shipping Method can only return a single price, a single option.
Describe the solution you'd like
There should be some method for a "ShippingMethod" to return multiple options and some way when selecting shipping to set not only which ShippingMethod but which of the options it provided to use.
On discord (https://discord.com/channels/1100672177260478564/1200213503139655750/1200340563724083290) the suggestion was proposed to allow a shipping calculator to return multiple prices in an attempt to make it a non-breaking change.
Describe alternatives you've considered
Two options I've thought about:
Shipping plugin provides custom APIs for getting shipping methods, adds custom fields to the order for which is selected. The ShippingCalculator then looks at those fields to decide if it's going to be used.
The shipping calculator return the available shipping options as metadata; again custom fields can be used to set which method to use but if not provided it can return some arbitrary (or arbitrarily chosen?) value.
Both are a bit hacky, I feel like 2 is hackier than 1 =]
Additional context
Here is an abbreviated example of what the Shipstation APIs might return. (not exact, but it's what my custom API returns after getting all available carriers and services)
The text was updated successfully, but these errors were encountered: