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

Update variant name, price and stock through the DFC API #12037

Merged
merged 6 commits into from
Jan 17, 2024

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jan 12, 2024

What? Why?

There were a few remaining fields left to import products via the DFC API. These have now been added with one exception. The issue also listed setting on_demand but that's currently not possible with the DFC Connector:

What should we test?

  • No test.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

A DFC SuppliedProduct relates to a Spree::Variant and when updating its
name we only want to change the name for that variant. Otherwise, when
we update the name of the product, it would update the name for all
variants and all the corresponding SuppliedProducts.
The DFC Connector doesn't support importing null values. This has to
wait until it's solved in the Connector or we have an urgent use case.
The DfcBuilder was doing everything to start with but we are moving its
parts to smaller modules now.
@mkllnk mkllnk added the api changes These pull requests change the API and can break integrations label Jan 12, 2024
@mkllnk mkllnk self-assigned this Jan 12, 2024
@mkllnk mkllnk marked this pull request as ready for review January 12, 2024 04:09
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good !

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Looks good to me, I just have a couple of questions going forward.

@@ -49,10 +49,10 @@ def self.import_product(supplied_product)

def self.apply(supplied_product, variant)
variant.product.assign_attributes(
name: supplied_product.name,
description: supplied_product.description,
Copy link
Member

Choose a reason for hiding this comment

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

Would this be a problem for description also?
Or do we need to keep this in order to achieve the current project..

Copy link
Member Author

Choose a reason for hiding this comment

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

Would this be a problem for description also?

Yes, variants don't have descriptions, only products do. And when you change it through one variant then all get changed. It's a limitation we can't avoid at the moment. Better than no description update at all.

context "with missing stockLimitation" do
let(:offer) {
offer_example.dup.tap do |o|
o.delete(:'dfc-b:stockLimitation')
Copy link
Member

Choose a reason for hiding this comment

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

Would the absence of a property imply a null value? That seems risky to me: if you sent just one property to be updated, you could inadvertently clear all others. I think it would need to explicitly send a null value.

I think we've had the discussion before, but I forget sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's not great. And I wouldn't apply this rule in general. But the DFC recommends this for stockLimitation, probably in absence of a better solution. There's no way to express infinity as real value in JSON.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could propose a new value, of course. Something like our on_demand flag. But that can then be confusing in combination with the stock limitation value. 🤷

@mkllnk mkllnk merged commit 12be3f9 into openfoodfoundation:master Jan 17, 2024
52 checks passed
@mkllnk mkllnk deleted the dfc-import branch January 17, 2024 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api changes These pull requests change the API and can break integrations
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Import product data from DFC via the DFC Connector
3 participants