Add default parcel size setting #3193
Conversation
- Create react component and container - Create a method for parcel-dimension - Set deafult parcel dimension sin cart.js - Add JSDoc
- Display toast message - Get default parcel size
…ther-fix-issue-3019
|
@zenweasel Should we take the extra time and convert this to React? Also, based on the title I don't think it's clear what this panel is meant to do. In terms of translations, this is too long but it should be something more like "Default parcel size." @spencern Do you have any ideas of a shorter clearer way to say that? |
|
Also, translations aren't working for the title.
|
|
One note based on that screen shot is that I don't think parcel size should have an "enable/disable" switch on it. Also have failing tests and a failing bithound file |
- Validate input - Refactor code
- Remove id check
…ther-fix-issue-3019
- Remove toggle button - Update translation - Update product schema - Update validation
|
@rymorgan I've fixed the translation issue. It now works. |
efalayi
changed the title
efalayi
changed the title
|
|
|
This should be 7 not 70
|
|
The weight isn't right on the product level.
|
|
@rymorgan the translation files have to be added in |
|
@Akarshit are you working on this presently? |
|
@efalayi The value is reaction/lib/collections/schemas/products.js Line 515 in 8338c21 |
|
@zenweasel yeah. That was a typo. It's undefined |
|
|
|
@zenweasel could you try creating a new product and the check the database? |
|
|
|
@efalayi Ya I will be working on this. |
|
Steps I followed for testing
Testing default value.
|
lib/collections/schemas/helpers.js
Outdated
| */ | ||
| weight: function () { | ||
| const shop = new shopDefaultParcelSize.currentShop(); | ||
| if (shop && shop.defaultParcelSize) { |
There was a problem hiding this comment.
@zenweasel could this be false anywhere?
There was a problem hiding this comment.
@Akarshit yes. It could be false if defaultParcelSize is not defined in Shops.json. After a discussion with @zenweasel we decided to also set autoValues for defaultParcelSize in shops.js.
server/methods/core/cart.js
Outdated
|
|
||
| // Check if any parcel property is set to zero and set to corresponding value on default parcel size | ||
| Object.keys(parcel).forEach(function (key) { | ||
| if (parcel[key] === 0) { |
There was a problem hiding this comment.
How can this be when in the schema it's defined to have minimum value of 1?
brent-hoover
force-pushed
the
esther-fix-issue-3019
branch
from
efb71a7
to
092c39e
Compare
|
@efalayi Sorry for the confusion. I am not working on this, please continue. |
|
@efalayi Before I am going to willing to approve this PR I really need to see some in-depth test instructions that cover all the various aspects and scenarios covered by this ticket (no values in Shop.json,etc. and non-US UOM). This has the potential to introduce critical bugs into checkout so I need to have some really good means for understanding when this is actually working correctly and not having an impact on anything else. Something along the lines of what @Akarshit had started there |
- Customise error messages - Refactor autoValue helper function - Allow zero value for product dimensions - Add autoValue for shop defaultParceSize - Fix bug in cart.js
…eaction into esther-fix-issue-3019
|
@zenweasel I just updated the branch. Test Steps
ObservationSome shipping providers have max values for parcel dimensions (for instance DHL). Suggestions
@Akarshit could you please help test this branch. Thank you. |
There was a problem hiding this comment.
See my inline comments. Also, I didn't see tests to make sure that default values are NOT used when individual product values are provided. But maybe an existing test covers that.
One big picture question: Shouldn't they all be optional: true in the schema? What if I wanted to provide a default weight only, but leave the others blank? I don't think the form will be valid without all 4 props having a value.
| this.handleFieldFocus = this.handleFieldFocus.bind(this); | ||
| this.handleStateChange = this.handleStateChange.bind(this); | ||
| this.handleSubmit = this.handleSubmit.bind(this); | ||
| this.handleCardExpand = this.handleCardExpand.bind(this); |
There was a problem hiding this comment.
All event handlers should be defined as arrow functions the way handleCardExpand is. Then you can delete all of these .bind calls from the constructor.
Also use props rather than this.props everywhere in the constructor.
| /** | ||
| * @name handleFieldFocus() | ||
| * @summary Handle input field focus in form | ||
| * @return {function} state for field value |
There was a problem hiding this comment.
Remove @return line from comment. It is not returning anything.
| * @name handleStateChange() | ||
| * @summary Handle input field change | ||
| * @param {script} event - onChange event when typing in input field | ||
| * @return {function} state for field value |
There was a problem hiding this comment.
Remove @return line from comment. It is not returning anything.
| event.preventDefault(); | ||
| const { size } = this.state; | ||
| const shopId = Reaction.getShopId(); | ||
| this.setState({ isSaving: true }); |
There was a problem hiding this comment.
Although you are calling setState here, no state changes ever happen until this function returns, which means that in an error case, this line is unnecessary.
I would rewrite the logic more like this:
handleSubmit: (event) => {
event.preventDefault();
const { saveDefaultSize, validation } = this.props;
const { size } = this.state;
const validationStatus = validation().validate(size);
if (!validationStatus.isValid) {
this.setState({ validationStatus });
return;
}
this.setState({ isSaving: true });
saveDefaultSize(Reaction.getShopId(), size, () => {
this.setState({
isSaving: false,
validationStatus: {}
});
});
};| * @return {Function} state for field value | ||
| */ | ||
| handleCardExpand = (event, card, cardName, isExpanded) => { | ||
| if (this.props.onCardExpand) { |
There was a problem hiding this comment.
You should destructure props at the top
const { onCardExpand, getEditFocus } = this.props;| "defaultParcelSize.length": size.length, | ||
| "defaultParcelSize.width": size.width, | ||
| "defaultParcelSize.height": size.height | ||
| } |
There was a problem hiding this comment.
Why not just
$set: { defaultParcelSize: size }| if (error) { | ||
| throw new Meteor.Error("server-error", error.message); | ||
| } | ||
| }); |
There was a problem hiding this comment.
It's not good for a method to throw after already returning. You should remove the callback and wrap in a try/catch instead.
| shopId = Reaction.getShopId(); | ||
| } else { | ||
| shopId = Reaction.getPrimaryShopId(); | ||
| } |
There was a problem hiding this comment.
I see this same code in 3 places already. Create a helper function they can all share:
function getShopIdCheckingMerchantCart() {
const marketplaceSettings = Reaction.getMarketplaceSettings();
if (marketplaceSettings && marketplaceSettings.public && marketplaceSettings.public.merchantCart) {
return Reaction.getShopId();
}
return Reaction.getPrimaryShopId();
}| decimal: true, | ||
| defaultValue: 6 | ||
| } | ||
| }); |
There was a problem hiding this comment.
When new SimpleSchema is merged, will need to remove decimal: true options.
Also add min: 0 option to all of them
I don't understand why the defaultValues are set as they are. I think this would confuse people. I'd set them all to 0, which will keep things unchanged from how it currently works
| weight: getDefaultParcelSize().weight, | ||
| length: getDefaultParcelSize().length, | ||
| width: getDefaultParcelSize().width, | ||
| height: getDefaultParcelSize().height, |
There was a problem hiding this comment.
These 4 lines can be more easily written as
...getDefaultParcelSize(),
|
@aldeed I think all four values are/were required because Shippo requires all four to give a shipping quote. |
|
@zenweasel But why do they need to be required in the defaults object? Example: I want to supply only a default weight, so I save the defaults form with only that field populated. It should succeed, resulting in defaults object of Then when I create a product, its I suppose making all required in the defaults object with default values of 0 is pretty much the same, however it results in more data being stored unnecessarily. |
|
@aldeed I guess we were thinking that the primary use case would be forcing someone to set these values to be non-zero so we are protecting them from failing during checkout. This is really mostly only relevant in the case of using Shippo though, when using Flat Rate (or other shippers that allow dimensional weight) it doesn't make sense to force anything but weight to be required and if you are using Flat Rate, you don't need any of these values. |
|
@zenweasel ah ok. Knowing that, I feel like there is something confusing about how this is implemented. You are entering these values in one place, but they're being used for two different purposes: (1) The defaults when you create a new product/variant, and (2) the In reality, I think one is more of a product setting (create each new product with these size values because they are common), and the other is more of a shipping setting (for checkout/processing, use these size values if a product does not have them, because shipping needs them). Should there be two different sets of defaults? Somewhat related, the docs and UI should be clearer about how inheritance works. If any one value is set on the variant, then it will take all 4 values from the variant rather than the parent, even if some are not set. It's possible the store owner expects per-field inheritance, and I didn't see any documentation about how it works. |
|
@aldeed Fair. Possibly one version of these settings could be moved into the Shippo settings (where we could force all four values to be set) and another in the "Catalog" section where they could all be optional. Also, I think that @spencern's original requirement was that there be a "default default" so that even if you didn't set the defaults there would still be some sort of value there. I think this is where we got into all the stuff with the Shop schemas. But maybe these can be set at the shipper level rather than at the shop? |
|
I've spoke with @spencern about this and although there's been some good work done here I am very tempted to close this PR and rethink the implementation from the ground up again. |
|
I am closing this PR pending a rethinking of how this should be implemented. This seemed like a simple enough change but was clearly not thought through enough (by me). |
Resolves #3019
Add a settings panel within the shipping dashboard that permits defining default parcel sizes for products and variants that don't have dimensions defined. Also include sensible defaults for the default parcel size so that it's never null which could cause Shippo or other shipment providers to fail to calculate shipping correctly.
Fix
reaction-shipping-parcel-sizeto manage parcel size settingShops.jsonwith default parcel size valuescart-create.app.testTest
meteor mongoand 'use meteor in terminal (or start Robomongo)db.Shops.find({}, {"defaultParcelSize": 1, "_id": 0})should give{ "defaultParcelSize" : { "weight" : 7, "length" : 11.25, "width" : 8.75, "height" : 6 } }Default parcel sizeform fields. They should be the same values in the databaseOrdersin database.items.parcelshould be the same value of product dimensionsOrdersin database and verify thatitems.parcelis equal to the new dimensions saved for the productitems.parcelshould be the same value of variant option dimensionsOrdersin database and verify thatitems.parcelis equal to the value for default parcel sizeShops.json