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

Stop setting line items with quantity of zero #5275

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RyanofWoods
Copy link
Contributor

Summary

In order to order to be able to update LineItem's validations to stop allowing a quantity of zero, we need to remove code that saves LineItems with a quantity of 0 (or less which gets normalized to 0).

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

Right now, when empty values are passed as quantity in the API call
that creates line items, the line item is populated with quantity 0.

This is not the expected behavior, as it makes no sense to have a line
item with 0 as quantity.

Still, if someone wants to force the value to 0, this is still possible
but 0 needs to be explicitly passed as quantity value. A spec has been
added for this scenario as well.
@github-actions github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem labels Jul 21, 2023
@RyanofWoods RyanofWoods force-pushed the ryanofwoods/stop-setting-line-items-with-quantity-of-zero branch from f996063 to ae87c41 Compare July 24, 2023 05:35
This method allows the deletion of LineItems where the quantity is set
to 0 or less. This is possible because LineItem allows a quantity of 0
on save, but soon this will be disallowed, so we have to manually
destroy these LineItems and remove them from the params before saving.
@RyanofWoods RyanofWoods force-pushed the ryanofwoods/stop-setting-line-items-with-quantity-of-zero branch from ae87c41 to 0d88daf Compare July 24, 2023 06:07
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I know it's a draft, but I was taking a look and left a couple of comments.

core/app/models/spree/order_contents.rb Show resolved Hide resolved
else
params[:line_items_attributes].reject! do |_index, attributes|
line_items_to_destroy_ids << attributes[:id] if attributes[:quantity].to_i <= 0
end
Copy link
Member

Choose a reason for hiding this comment

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

In which cases do we have this difference in passing the attributes?

Copy link
Contributor Author

@RyanofWoods RyanofWoods Jul 24, 2023

Choose a reason for hiding this comment

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

Do you mean whether it's nested or not? Like so:

# Case 1:
line_items_attributes: {
  id: 1,
  quantity: 1
}

# Case 2:
line_items_attributes: {
  "01" => {
    id: 1,
    quantity: 1
  },
  "02" => {
    id: 3,
    quantity: 0
  }
}

I am supporting single nesting for Api::LineItemsController, though it seems that Rails supports these different levels by default - though I couldn't find any explicit documentation about it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Hey @RyanofWoods, I'm not able to detect any code in the Api::LineItemsController or its specs that seem to support this way of passing attributes. Can you please help me adding more details about your discovery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case 1 is needed for Spree::Api::LineItemsController:

def line_items_attributes
{ line_items_attributes: {
id: params[:id],
quantity: params[:line_item][:quantity],
options: line_item_params[:options] || {}
} }
end

Case 2 was needed for how the OrderContents spec is setup.

Both these formats are supported by the nested_attributes in Rails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants