Prevent admins to be able to view carousel in shops by chahmedejaz · Pull Request #14364 · openfoodfoundation/openfoodnetwork

chahmedejaz · 2026-06-02T23:38:36Z

What? Why?

Closes Prevent admins to be able to display carousel in shops #14157

As part of the multiple product images roadmap, the carousel functionality is being developed before backoffice support for managing multiple images is available.

Currently, it is possible for products to have multiple images associated with them through the image settings page, even though this functionality is not intended to be available to users yet. To keep production data aligned with the current user experience, this PR:

Adds a data migration that removes secondary images from products, ensuring each product has at most one product-level image.
Removes the ability to create additional product images from the product's image page by hiding the "New Image" action.

Users can still replace an existing product image through the product list and image settings page, so there is no change to the intended image management workflow. A new image can only be added through the product list page.

What should we test?

Scenario 1 - Existing product image management

Create or open a product that already has an image.
Navigate to the product's Images page.
Verify there is no New Image button.
Edit the existing image.
Upload a replacement image and save.
Verify the image is updated successfully.

Scenario 2 - Product without an image

Create or open a product with no image.
Navigate to the product's Images page.
Verify the "no images found" state is displayed.
Verify there is no option to create an additional image from this page.
From the product list, upload an image for the product.
Verify the image appears correctly on the Images page.

Scenario 3 - Migration

Run the migration against data containing products with multiple product images.
Verify that only the first product image record is retained. i.e. There should not be any visual change of image for the product in the app before and after the migration.
Verify that additional product images are removed.

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

Dependencies

Need the following PR to be merged along with this one:

Show carousel of images when multiple images are available #14337

rioug

It looks good, I just have some reservation around the migration testing.

rioug · 2026-06-05T00:02:51Z

+    let(:product) { create(:product) }
+
+    it 'keeps the first image and removes additional images for a product' do
+      first_image = described_class::SpreeImage.create!(


Shouldn't we be using the actual app model instead of the one defined in the migration ? I think technically it should not make a difference but I feel like we are not testing a real production scenario where database entries would be created by a different model. Thought ?

Good point, @rioug. I considered using the application Spree::Image model initially as well, but I ended up avoiding it because migrations should ideally be tested independently of the application code.
But your point is valid as well. That way, we may not be testing the actual production scenario. I've addressed it here: b7f8de2

rioug

Nice ! thanks for updating the migration spec 👍

mkllnk

Nice. Ideally we would modify the UX first and then delete old data in a separate pull request to avoid uploads during deploy. But it's very unlikely. So this is okay.

I have one suggestion below but you can also just leave it as is. Not important.

mkllnk · 2026-06-05T02:10:36Z

+
+RSpec.describe EnsureSingleProductImage, type: :migration do
+  let(:migration) { described_class.new }
+  let(:attachment) { Rack::Test::UploadedFile.new(Rails.root.join('app/webpacker/images/logo-white.png'), "image/png") }


We have a helper for this:

Suggested change

let(:attachment) { Rack::Test::UploadedFile.new(Rails.root.join('app/webpacker/images/logo-white.png'), "image/png") }

include FileHelper

let(:attachment) { white_logo_file }

mariocarabotta · 2026-06-07T12:38:30Z

@chahmedejaz
In the description,what do you mean by "Verify that only the earliest product image is retained"?
I might have not made it clear in the original issue, but I believe the image we want to keep is the latest one uploaded, the most recent - maybe that's what you mean by earliest? This is because anytime a new image gets uploaded in the Images section, it's the one that gets displayed in the frontshop.

Also, just wondering: are you planning to directly delete the images or they're still going to be retained for a while in case for very unlikely reasons some revert is needed?

Thank you!

chahmedejaz · 2026-06-07T14:51:12Z

Hey @mariocarabotta - you're right, but that's the behavior when uploading an image from the Products listing page, where the upload updates the earliest image record.

By "Verify that only the earliest product image is retained", I was referring to the multiple images created when using the New Image button on the Images page. In that scenario, only the first image is actually displayed.

Sorry for the confusion - I just realized the wording is misleading. I'll update the testing instructions. Thanks for pointing it out!

Also, just wondering: are you planning to directly delete the images or they're still going to be retained for a while in case for very unlikely reasons some revert is needed?

Hmm... that's a good callout. My initial thought was to remove the image records that aren't actually being displayed by the application. Before doing that, though, we could take a database backup so we have an easy rollback path if we need to restore anything later.

Edit:
I've updated the instructions with the following:
Verify that only the first product image record is retained. i.e. There should not be any visual change of image for the product in the app before and after the migration.
Please review it looks good. Thanks!

mariocarabotta · 2026-06-08T02:06:38Z

thanks @chahmedejaz for clarifying, that was actually my mistake, and your approach is correct.
Just to double check, a few simple tests I've done. These are done without the carousel being staged.

Add first image to product

Create product
Add image A
product displays image A

Add new image from settings

image A currently displayed
add image B by clicking button on Image settings and save
image A is still displayed >>> you are correct, the earliest image is retained as primary

Replace image

click on image in product list
upload image file C
product displays new file C, but it's still a single image record (one image in Images settings)
Same behaviour happens if I replace the file from the Imags settings

I would still suggest to do a snapshot of the database for backup. Since this is a bug and I am not entirely sure what the software was doing in the past (and why those leftovers are still there :D ), it feels a bit safer to keep the data for a while.

Thank you again!

chahmedejaz · 2026-06-08T08:14:19Z

Thanks @mariocarabotta for the detailed testing. And sure we will make sure to keep the snapshot of the database just in case.

AMEA-LYON · 2026-06-08T09:01:11Z

Tested successfully scenario 1 & 2...
How do I proceed to test migration scenario ?

chahmedejaz · 2026-06-08T12:52:37Z

Thanks @AMEA-LYON - For the migration scenario, we just need to make sure that before and after the PR the visible images on the products should remain intact. For example we added multiple images on a product. And only the first one is visible on the page. After the migration, the first image should still be visible.

Deletion of images can only be verified when its dependent PR is merged. So, I think we can verify this scenario in the release testing.
Please let me know if you have any questions on this. Thanks

AMEA-LYON · 2026-06-08T14:49:56Z

Thanks for the explanations @chahmedejaz ,

so I deployed 5.5.0, and added 2 images on a product,
when I deployed the PR, the first image was still visible.
I then deleted the image, the second one was still present... You will have to check after deplying the related PR...

chahmedejaz · 2026-06-09T00:20:35Z

Sure @AMEA-LYON - I'll check this scenario and get back to you. Thanks!

chahmedejaz · 2026-06-09T00:40:17Z

@openfoodfoundation/reviewers I just thought of something:

If we destroy a product image, would that also delete the underlying file from S3? I assume it would, since I verified locally that the file gets removed from the filesystem when the image record is destroyed.
If that's the case, then taking a database backup alone may not be sufficient for rollback/revert scenarios, because the image files themselves would already be gone.
The image model also doesn't use paranoid, so soft deletion isn't currently available.
Would adding soft deletion to the image model address this concern? Or is there another approach we should consider for preserving images during a rollback?
Lastly, I really want to test the rollback functionality in staging just to be on the safer side. Do we have any guideline to restore the database backup?

rioug · 2026-06-09T01:06:46Z

I would assume the underlying Image in S3 would be deleted, you should be able to check that in staging.
I am not sure it makes sense to use paranoia for images, but that might be good short term solution to safely enable testing your changes. I am not sure every instance uses S3 to store images, I believe some just use the disk, so any rollback solution would need to account for every image storage scenario. So, I think the easiest solution would to use paranoia as you suggest, and once we are certain we don't need old images any more we can just delete them and remove paranoia in a later PR.

Lastly, I really want to test the rollback functionality in staging just to be on the safer side. Do we have any guideline to restore the database backup?

There is something in place to allow you to backup the database before releasing a PR on staging, which you can then load back on staging. But I can't remember how it works, maybe @mkllnk or @RachL would know more.

RachL · 2026-06-09T07:27:28Z

i never used it, but Filipe has documented some steps here, maybe it helps: https://ofn-user-guide.gitbook.io/ofn-testing-handbook/testers-toolkit/reverting-database-changes

chahmedejaz · 2026-06-10T00:08:14Z

Thanks both, it really helps alot!

…ity and consistency

This commit needs to be reverted once we are good with the migration.

chahmedejaz · 2026-06-16T00:24:36Z

@rioug - I've added the soft delete functionality in this commit. To be honest, I don't particularly like this workaround, but after investigating ActiveStorage's behavior, this seemed to be the cleanest and safest approach.

The issue is that even though Spree::Image is soft-deleted via Paranoia, destroying the image record still removes the associated ActiveStorage attachment and underlying file. To preserve files for soft-deleted images, image updates now create a replacement image and only soft-delete the previous one after a successful save. The attachment association's dependent behavior has also been overridden to prevent automatic file cleanup.

This is intended to be a temporary solution, and we should be able to revert this specific commit once we get confirmation from instance managers that all existing images are intact and no further recovery actions are required.

rioug

Thanks @chahmedejaz, I think we need to fix the migration order, but we should be good afterwards.

rioug · 2026-06-16T00:52:33Z

      image_to_keep = images.first

-      images.where.not(id: image_to_keep.id).delete_all
+      images.where.not(id: image_to_keep.id).update_all(deleted_at: Time.current)


This needs to run after the other migration, and I am pretty sure currently it's the other way around, based on the migration's timestamp in the name. Could you fix the migration to make sure they run in the correct order ?

rioug · 2026-06-16T00:59:18Z


+    attachment_reflection = _reflections["attachment_attachment"]
+    attachment_reflection.options[:dependent] = nil
+


I am assuming this refers to :

The attachment association's dependent behavior has also been overridden to prevent automatic file cleanup.

Could you add a comment to explain what it does and why it's needed ?

github-project-automation Bot added this to OFN Delivery board Jun 2, 2026

github-project-automation Bot moved this to All the things 💤 in OFN Delivery board Jun 2, 2026

github-actions Bot assigned chahmedejaz Jun 2, 2026

chahmedejaz mentioned this pull request Jun 2, 2026

Show carousel of images when multiple images are available #14337

Draft

4 tasks

chahmedejaz force-pushed the task/14157-prevent-admin-from-adding-multiple-images branch 3 times, most recently from 73a5fe9 to 17d5bcb Compare June 3, 2026 20:25

chahmedejaz moved this from All the things 💤 to In Progress ⚙ in OFN Delivery board Jun 3, 2026

chahmedejaz moved this from In Progress ⚙ to Code review 🔎 in OFN Delivery board Jun 3, 2026

chahmedejaz added dependencies user facing changes Thes pull requests affect the user experience labels Jun 3, 2026

chahmedejaz marked this pull request as ready for review June 3, 2026 22:46

rioug reviewed Jun 5, 2026

View reviewed changes

chahmedejaz requested a review from rioug June 5, 2026 00:51

rioug approved these changes Jun 5, 2026

View reviewed changes

mkllnk approved these changes Jun 5, 2026

View reviewed changes

mkllnk moved this from Code review 🔎 to Test Ready 🧪 in OFN Delivery board Jun 5, 2026

AMEA-LYON self-assigned this Jun 8, 2026

AMEA-LYON added the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 8, 2026

AMEA-LYON removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 8, 2026

chahmedejaz added the feedback-needed label Jun 9, 2026

chahmedejaz removed the feedback-needed label Jun 11, 2026

chahmedejaz added 3 commits June 15, 2026 06:16

Add migration to ensure single product image and corresponding tests

e2db4f8

remove new image action button

ac0dfcd

Refactor migration tests to use Spree::Image directly for better clar…

ea26d55

…ity and consistency

chahmedejaz force-pushed the task/14157-prevent-admin-from-adding-multiple-images branch 3 times, most recently from edecf47 to 3b5622d Compare June 15, 2026 23:49

chahmedejaz moved this from Test Ready 🧪 to In Progress ⚙ in OFN Delivery board Jun 15, 2026

chahmedejaz force-pushed the task/14157-prevent-admin-from-adding-multiple-images branch from 3b5622d to 1c3ad26 Compare June 16, 2026 00:06

Add soft delete functionality.

fba566f

This commit needs to be reverted once we are good with the migration.

chahmedejaz force-pushed the task/14157-prevent-admin-from-adding-multiple-images branch from 1c3ad26 to fba566f Compare June 16, 2026 00:09

chahmedejaz requested a review from rioug June 16, 2026 00:24

chahmedejaz moved this from In Progress ⚙ to Code review 🔎 in OFN Delivery board Jun 16, 2026

rioug requested changes Jun 16, 2026

View reviewed changes

github-project-automation Bot moved this from Code review 🔎 to In Progress ⚙ in OFN Delivery board Jun 16, 2026

	let(:attachment) { Rack::Test::UploadedFile.new(Rails.root.join('app/webpacker/images/logo-white.png'), "image/png") }
	include FileHelper
	let(:attachment) { white_logo_file }


		attachment_reflection = _reflections["attachment_attachment"]
		attachment_reflection.options[:dependent] = nil

Uh oh!

Conversation

chahmedejaz commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What? Why?

What should we test?

Scenario 1 - Existing product image management

Scenario 2 - Product without an image

Scenario 3 - Migration

Release notes

Dependencies

Uh oh!

rioug left a comment

Choose a reason for hiding this comment

Uh oh!

rioug Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

chahmedejaz Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

rioug left a comment

Choose a reason for hiding this comment

Uh oh!

mkllnk left a comment

Choose a reason for hiding this comment

Uh oh!

mkllnk Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

mariocarabotta commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chahmedejaz commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariocarabotta commented Jun 8, 2026

Uh oh!

chahmedejaz commented Jun 8, 2026

Uh oh!

AMEA-LYON commented Jun 8, 2026

Uh oh!

chahmedejaz commented Jun 8, 2026

Uh oh!

AMEA-LYON commented Jun 8, 2026

Uh oh!

chahmedejaz commented Jun 9, 2026

Uh oh!

chahmedejaz commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rioug commented Jun 9, 2026

Uh oh!

RachL commented Jun 9, 2026

Uh oh!

chahmedejaz commented Jun 10, 2026

Uh oh!

chahmedejaz commented Jun 16, 2026

Uh oh!

rioug left a comment

Choose a reason for hiding this comment

Uh oh!

rioug Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

rioug Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chahmedejaz commented Jun 2, 2026 •

edited

Loading

mariocarabotta commented Jun 7, 2026 •

edited

Loading

chahmedejaz commented Jun 7, 2026 •

edited

Loading

chahmedejaz commented Jun 9, 2026 •

edited

Loading