Create sourced variants by dacook · Pull Request #13912 · openfoodfoundation/openfoodnetwork

dacook · 2026-02-10T02:13:17Z

ℹ️ Please use Clockify code #76a Sourced variant for review and testing.

What? Why?

Closes As an enterprise user, I want to create a sourced variant from products I manage for another producer #13887

What should we test?

1. Set up permission

User has an enterprise (is manager or owner) that has some products
There is also a "supplier" enterprise with at least one product
Grant enterprise permission from suppler to user enterprise: can manage products and create sourced variants

2. Create sourced variants

Go to admin products screen
My enterprise and supplier's products should appear
Click actions menu ⋮ on my products: no option to create sourced variant
Click actions menu ⋮ on supplier's products: then Create sourced variant

Sourced variant is created:

all details have been cloned
a ⛓️‍💥 icon should appear. Hover over to see which variant it is linked to, and that it is owned by my enterprise.

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

This feature is a work in progress and may be toggled on by the use of Enterprise Permissions as described above.

These tests are about browsing products, not performing actions. Well, ok there's one about updating, which should probably go in the update file. But hey this is better than before. And admittedly the "Actions" file covers three different things, not just the actions menu. shrug.

There's no "COPY OF" product in the spec setup, so we don't need to check that it's not there. (unless maybe we added that to the product factory, but it seems unlikely). Also we can use helper method.

Each context has different setup and needs to load the page after setup.

For now, we will only be able to create sourced variant from variants that are visible to us (variants that we manage) In a later commit I will hide the option if you can't use it

The permission is effectively the feature toggle. Users can choose to use it, but shouldn't expect it all to work perfectly yet. When it's considered full featured, we just need to update the translation. Hm... I hope that's not too painful.🤞

Tried using the rails generator, but as usual it was a waste of time becuase it doesn't handle unusual cases. I found more good guidance from that stackoverflow post: > why are you worrying about your indexes? Build your app! Something's not right in the model, see next commit.

Co-authored-by: dacook <4188088+dacook@users.noreply.github.com> Thanks co-pilot for sending me in the right direction. Would this be neater as a has_and_belongs_to_many? Maybe but I will try to keep moving.

It's quite ugly. But we will be iterating on this later.

TIL we have linting on haml. I couldn't think of a better way to handle this but would be glad to receive feedback.

Preload the allow list once in the controller. This controller was initially set up to avoid instance variables, and pass variables explicitly to the template. That's a good principle, but in practice we have a growing list of variables passed down the chain to multiple partials which is getting cumbersome. I think instance variables have their place after all.

Should existing variants be migrated to have an owner (copied from supplier)? No, because you can change supplier. This concept needs work.

I tried to avoid it but rubocop made me move it. I think maybe it will need to go into a concern or service class later, but hopefully it's ok here for now.

mkllnk

Nice commit history! It was really easy to follow.

I would almost approve but I think that the migration is worth changing before merging this. And the rest of my comments, ... up to you.

mkllnk · 2026-03-04T05:12:06Z

app/controllers/admin/products_v3_controller.rb

+        variant.save!
+        variant.on_demand = source_variant.on_demand
+        variant.on_hand = source_variant.on_hand
+        variant.save!


You don't need to save before you can set stock levels. You just need to build the stock item as well:

openfoodnetwork/engines/dfc_provider/app/services/catalog_item_builder.rb

Lines 21 to 38 in 59547ba

def self.apply_stock(item, variant)

limit = item&.stockLimitation

return if limit.blank?

if variant.stock_items.empty?

variant.stock_items << Spree::StockItem.new(

variant:,

)

end

if limit.to_i.negative?

variant.stock_items[0].backorderable = true

else

variant.stock_items[0].backorderable = false

variant.stock_items[0].count_on_hand = limit

end

end

I think that we should make this easier in our application and maybe create the stock item on the fly...

Also, we still have the has_many :stock_items relationship when we just have one associated...

app/models/variant_link.rb

db/schema.rb

app/models/spree/variant.rb

mkllnk · 2026-03-04T05:49:57Z

app/models/spree/variant.rb

    belongs_to :shipping_category, class_name: 'Spree::ShippingCategory', optional: false
    belongs_to :primary_taxon, class_name: 'Spree::Taxon', touch: true, optional: false
    belongs_to :supplier, class_name: 'Enterprise', optional: false, touch: true
+    belongs_to :owner, class_name: 'Enterprise', optional: true


This is slightly confusing. The Enterprise has an owner of type User and the variant now has an owner of type Enterprise. But I don't have a better idea...

Good point. Perhaps we could name it owner_enterprise or owning_enterprise. Will think about that.

I've opened up the conversation in slack: https://openfoodnetwork.slack.com/archives/C0A9UJ8GVA7/p1773123856656439
Maybe I was thinking too much about it..

db/migrate/20260225022934_add_owner_to_spree_variants.rb

mkllnk · 2026-03-04T05:57:01Z

app/models/spree/variant.rb

+      owner_id = EnterpriseRelationship.permitted_by(supplier).permitting(user.enterprises)
+        .with_permission(:create_sourced_variants)
+        .pick(:child_id)


Hm, what if there are multiple enterprises? I guess we solve that later? Or you can re-assign the owner later, maybe?

Yeah, we discussed adding a selector for the user to choose on creation. I'm not sure but Mario may be working this into a future issue.

app/models/spree/variant.rb

mkllnk · 2026-03-04T06:02:40Z

app/models/spree/variant.rb

+        variant.price = price
+        variant.save!
+        variant.source_variants = [self]
+        variant.owner_id = owner_id


It does make sense to have the method here. It's mostly referencing the variant. It's a bit like to_s or dup.

These records won't be updatable, but it might still be worth tracking when they were created.

I guess my local db is slightly out of sync.

I think I was just following convention from existing relationships. Perhaps you could argue that a variant is affected by the links added/removed.. but we never look at updated_at so really there's no point at all.

For uniformity Co-authored-by: Maikel <maikel@email.org.au>

dacook · 2026-03-10T03:59:28Z

app/controllers/admin/products_v3_controller.rb

+      rescue ActiveRecord::RecordInvalid
+        flash.now[:error] = variant.errors.full_messages.to_sentence
+        status = :unprocessable_entity
+        variant_index = "-1" # Create a unique-enough index
+      end


undercover: 👮‍♂️ some methods have no test coverage! Please add specs for methods listed below

It's true, this is a theoretical case. I could probably mock it in a spec to test that it's working.
But you could also argue that if it's not a known possibility, the code shouldn't be there.
In this case, I argue that being ready to show the error to the user is useful enough to warrant keeping the code .(maybe one day an unexpected change will cause it to fail, and the reason will be displayed to help debug immediately).
🤷

dacook · 2026-03-10T04:43:24Z

I'm going to try renaming "Sourced variants" to "linked variants" next.. 😬

Actually, the variant factory is still adding an extra save. We should refactor Variant to avoid that.. but the afternoon slump has got me.

There are two types of linked variant associations: source and target, so we need to keep the name there. But when cloning a variant and retaining a link as source, we will prefer the general term 'linked variant'. Hopefully this name works well.

dacook self-assigned this Feb 10, 2026

github-project-automation bot added this to OFN Delivery board Feb 10, 2026

github-project-automation bot moved this to All the things 💤 in OFN Delivery board Feb 10, 2026

sigmundpetersen moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Feb 10, 2026

dacook force-pushed the sourced-variant1-13887 branch 2 times, most recently from 8d99662 to cff3cc3 Compare February 17, 2026 02:59

dacook force-pushed the sourced-variant1-13887 branch 2 times, most recently from 14f37d6 to f9549bb Compare March 3, 2026 06:16

dacook force-pushed the sourced-variant1-13887 branch from f9549bb to 855180d Compare March 4, 2026 01:35

dacook and others added 14 commits March 4, 2026 12:36

Clean up spec

b4bf5d3

There's no "COPY OF" product in the spec setup, so we don't need to check that it's not there. (unless maybe we added that to the product factory, but it seems unlikely). Also we can use helper method.

Remove unnecessary before action

665148a

Each context has different setup and needs to load the page after setup.

Add 'create sourced variant' option in variant actions menu

ab80721

For now, we will only be able to create sourced variant from variants that are visible to us (variants that we manage) In a later commit I will hide the option if you can't use it

Add enterprise permission create_sourced_variants

5a6b5e6

Label this feature as 'beta'

ad54967

The permission is effectively the feature toggle. Users can choose to use it, but shouldn't expect it all to work perfectly yet. When it's considered full featured, we just need to update the translation. Hm... I hope that's not too painful.🤞

Set up permissions for creating source variants

8cf55ce

Create source variants

9a58240

Fix source_variants and target_variants associations in Variant model

924a7b6

Co-authored-by: dacook <4188088+dacook@users.noreply.github.com> Thanks co-pilot for sending me in the right direction. Would this be neater as a has_and_belongs_to_many? Maybe but I will try to keep moving.

Create sourced variant link on clone

d6252aa

Display presence of variant link in UI

e019e8d

It's quite ugly. But we will be iterating on this later.

haml-lint:disable ViewLength

bbf744a

TIL we have linting on haml. I couldn't think of a better way to handle this but would be glad to receive feedback.

dacook force-pushed the sourced-variant1-13887 branch from 855180d to 1e4e064 Compare March 4, 2026 01:51

dacook added 3 commits March 4, 2026 13:02

AddOwnerToSpreeVariants [migration]

38e0b7c

Should existing variants be migrated to have an owner (copied from supplier)? No, because you can change supplier. This concept needs work.

Set owner enterprise when creating source variant

d2a83e0

Refactor: move variant duplication to model

b7e3f42

I tried to avoid it but rubocop made me move it. I think maybe it will need to go into a concern or service class later, but hopefully it's ok here for now.

dacook force-pushed the sourced-variant1-13887 branch from 1e4e064 to b7e3f42 Compare March 4, 2026 02:02

dacook moved this from In Progress ⚙ to Code review 🔎 in OFN Delivery board Mar 4, 2026

dacook marked this pull request as ready for review March 4, 2026 02:40

mkllnk requested changes Mar 4, 2026

View reviewed changes

github-project-automation bot moved this from Code review 🔎 to In Progress ⚙ in OFN Delivery board Mar 4, 2026

dacook and others added 5 commits March 10, 2026 14:03

Avoid useless updated_at column

d46531e

These records won't be updatable, but it might still be worth tracking when they were created.

Revert uninentional schema change

d3fb86b

I guess my local db is slightly out of sync.

Remove touch

45b77e8

I think I was just following convention from existing relationships. Perhaps you could argue that a variant is affected by the links added/removed.. but we never look at updated_at so really there's no point at all.

Use reference command

571fc76

For uniformity Co-authored-by: Maikel <maikel@email.org.au>

ORder by ID to ensure deterministic results

1d024e6

dacook commented Mar 10, 2026

View reviewed changes

dacook force-pushed the sourced-variant1-13887 branch from fa9b2a9 to 6619a1a Compare March 10, 2026 04:41

dacook added 2 commits March 10, 2026 15:44

Move class instance variable to helper

fdf951a

Avoid unnecessary save

37da6d3

Actually, the variant factory is still adding an extra save. We should refactor Variant to avoid that.. but the afternoon slump has got me.

dacook force-pushed the sourced-variant1-13887 branch 2 times, most recently from cfcb610 to dc603fc Compare March 10, 2026 06:02

dacook force-pushed the sourced-variant1-13887 branch from dc603fc to 6b2bdf5 Compare March 10, 2026 06:28

	def self.apply_stock(item, variant)
	limit = item&.stockLimitation

	return if limit.blank?

	if variant.stock_items.empty?
	variant.stock_items << Spree::StockItem.new(
	variant:,
	)
	end

	if limit.to_i.negative?
	variant.stock_items[0].backorderable = true
	else
	variant.stock_items[0].backorderable = false
	variant.stock_items[0].count_on_hand = limit
	end
	end

Uh oh!

Conversation

dacook commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What? Why?

What should we test?

1. Set up permission

2. Create sourced variants

Release notes

Uh oh!

mkllnk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dacook commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dacook commented Feb 10, 2026 •

edited

Loading