Deprecate creating new shipment with an item via API #4387

kennyadsl · 2022-05-30T10:47:34Z

Description

There's no known reason why we need to force creating a shipment with exactly one variant into it.

There's a dedicated endpoint for that:

PUT /api/shipments/{shipment_number}/add

This feature is not used anymore in our backend UI and, if someone is using this endpoint, they will receive a deprecation warning with the instructions on how to adjust their flow to be compatible with the next major version.

TODO:

deprecate fields in the related API documentation endpoint

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
~~I have attached screenshots to this PR for visual changes (if needed)~~

This change is needed to allow deprecating passing the variant (and quanityt) in this endpoint.

There's no known reason why we need to force creating a shipment with exactly one variant into it. There's a dedicated endpoint for that, see the deprecation warning message in this commit for details.

The reload is probably there because, after adding some items to the order, the in-memory shipment object may be outdated. In case no item is added, there's no need to perform an extra query.

This commit move the generic specs in the outer context and keeps the one related to the deprecated variant_id params in its own context. It will be easier to remove them when it's time to remove deprecated code. This commit also removes a code comment that at the moment doesn't provide a lot of value.

There are a couple of untested scenarios now and this should cover this area well.

Due to the previous changes, we need to be sure that adding a product to an empty shipment works correctly. This was never done before because it wasn't possible to crete an empty shipment, so a new test is required.

waiting-for-dev · 2022-05-30T11:41:52Z

api/app/controllers/spree/api/shipments_controller.rb

+          variant = Spree::Variant.unscoped.find(params[:variant_id])
+          @order.contents.add(variant, quantity, { shipment: @shipment })
+          @shipment.save!
+          @shipment.reload


Do we need reload here?

I don't know (see d018c6e). But being code that we are going to remove soon, I think it's not even worth spending too much time to understand if there was a reason for that reload to be there. If you think we need to investigate more, I'll happily commit to that.

Yeah, you're right 👍

kennyadsl · 2022-05-30T14:34:05Z

api/openapi/solidus-api.oas.yml

+
+        **Deprecation Warning**: Adding items to the shipment via this endpoint
+        is deprecated. Instead, create an empty shipment and populate it with
+        the dedicated endpoint [to add items to the shipment](/docs/solidus/7078dbcf415ac-add-shipment-item).


I don't know if this way of adding link will work. I saw the docs here but I don't know how to test it until it's published.

Oh, interesting. I was looking for something like it for #4385 & #4388. If it works, I'll update the references I added there.

It worked! 🥳 https://solidus.stoplight.io/docs/solidus/branches/master/6e41c2d519a2b-create-shipment

@waiting-for-dev yep, but does it switch from the current branch to the main one? I don't think there's a way to capture the current branch information in the link anyhow.

What do you mean? The deprecation warning is only rendered on master, and I can switch the version with the top-left selector without problems.

I submitted #4397 to update my references.

I mean that if you click on the link from the master version, it will move you to the default one (currently 3.1) because the /branches/master/ part of the URL is not reflected in the link.

Oh, I see 🤦

still better than nothing 🤷

Yeah, I thought exactly that 😉

waiting-for-dev

Thanks!!

spaghetticode

Looks good to me, thank you!

spaghetticode · 2022-05-31T07:16:21Z

api/app/controllers/spree/api/shipments_controller.rb

@@ -41,6 +41,8 @@ def select_shipping_method
      def create
        authorize! :create, Shipment
        quantity = params[:quantity].to_i
+        variant = Spree::Variant.unscoped.find(params[:variant_id])


Just for curiosity, do you know why we needed to unscope variants here?

Good point. It was added here, it seems to allow removing discarded items from a shipment. It doesn't seem necessary for adding them though, changing this, thanks for the question, Andrea!

Wait, thinking twice, I'd prefer not to make that change. For the same reason described on another commet, I think changing this deprecated code now could give us more pain than benefits. Are you ok keeping this as is?

That makes sense! Thanks for double-checking this.

…ipments Now you should create a shipment and after that, use the dedicated endpoint to add stuff to it: PUT /api/shipments/{shipment_number}/add Ref solidusio#4387

Now you should create a shipment and after that, use the dedicated endpoint to add stuff to it: PUT /api/shipments/{shipment_number}/add Ref #4387

kennyadsl added 6 commits May 30, 2022 12:22

Allow creating a shipment via api without passing a variant

796bbb4

This change is needed to allow deprecating passing the variant (and quanityt) in this endpoint.

Deprecate passing variant_id and quantity when creating shipments

472817c

There's no known reason why we need to force creating a shipment with exactly one variant into it. There's a dedicated endpoint for that, see the deprecation warning message in this commit for details.

Reload @shipment only when it's changed

d018c6e

The reload is probably there because, after adding some items to the order, the in-memory shipment object may be outdated. In case no item is added, there's no need to perform an extra query.

Add more specs for shipment creation deprecated params

19fc818

There are a couple of untested scenarios now and this should cover this area well.

Add specs to test adding items to empty shipments

6127486

Due to the previous changes, we need to be sure that adding a product to an empty shipment works correctly. This was never done before because it wasn't possible to crete an empty shipment, so a new test is required.

kennyadsl self-assigned this May 30, 2022

kennyadsl added the Needs Work label May 30, 2022

waiting-for-dev reviewed May 30, 2022

View reviewed changes

Mark variant_id and quantity as deprecated creating api shipments

6b8004b

kennyadsl commented May 30, 2022

View reviewed changes

kennyadsl added changelog:solidus_api Changes to the solidus_api gem and removed Needs Work labels May 30, 2022

kennyadsl marked this pull request as ready for review May 30, 2022 14:51

waiting-for-dev approved these changes May 31, 2022

View reviewed changes

spaghetticode approved these changes May 31, 2022

View reviewed changes

kennyadsl merged commit a5e184d into solidusio:master May 31, 2022

kennyadsl deleted the kennyadsl/api/deprecate-creating-shipment-with-items branch May 31, 2022 11:25

waiting-for-dev mentioned this pull request Jun 1, 2022

Make API docs deprecation warnings consistent #4397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate creating new shipment with an item via API #4387

Deprecate creating new shipment with an item via API #4387

kennyadsl commented May 30, 2022 •

edited

Loading

waiting-for-dev May 30, 2022

kennyadsl May 30, 2022 •

edited

Loading

waiting-for-dev May 31, 2022

kennyadsl May 30, 2022

waiting-for-dev May 31, 2022

waiting-for-dev Jun 1, 2022

kennyadsl Jun 1, 2022

waiting-for-dev Jun 1, 2022

kennyadsl Jun 1, 2022

waiting-for-dev Jun 1, 2022

kennyadsl Jun 1, 2022

waiting-for-dev Jun 1, 2022

waiting-for-dev left a comment

spaghetticode left a comment

spaghetticode May 31, 2022

kennyadsl May 31, 2022

kennyadsl May 31, 2022

spaghetticode May 31, 2022

Deprecate creating new shipment with an item via API #4387

Deprecate creating new shipment with an item via API #4387

Conversation

kennyadsl commented May 30, 2022 • edited Loading

Choose a reason for hiding this comment

kennyadsl May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented May 30, 2022 •

edited

Loading

kennyadsl May 30, 2022 •

edited

Loading