[BUU] Add option to delete a product or variant #11846

chahmedejaz · 2023-11-22T23:27:01Z

What? Why?

Closes [BUU] Delete a product or variant #11068
Implemented the delete action feature for the products and variants in Backoffice AdminV3

What should we test?

All the scenarios mentioned in the linked issue

Change log Category:

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

dacook · 2023-12-05T03:26:40Z

Hi @chahmedejaz , looks like you've done lots of work on this, which is great. I was just wondering if you could give a rough indication of how long until it will be ready for review?
Thanks in advance.

chahmedejaz · 2023-12-05T05:38:31Z

Hi @chahmedejaz , looks like you've done lots of work on this, which is great. I was just wondering if you could give a rough indication of how long until it will be ready for review?
Thanks in advance.

I'm almost done with it.
Just need to create a reflex action to delete variants and the test cases. Maybe a day or 2 max.
Except for those, the user feedback message will remain.

P.S. Sorry for taking this long, actually got sick in between and couldn't work for a good chunk.😅

dacook · 2023-12-05T06:34:00Z

That's ok, sorry to hear you've been sick, I hope you're feeling better soon.

Thanks for trying out a reflex action, then we can use the loading indicator consistently 👍

dacook

Hi Ahmed, I've reviewed and overall I think you've done a great job pulling all this together into a workable solution.

I have a few suggestions for changes (those marked with ±), can you please review these and let me know what you think?

Also one more thing: you don't need to add the issue ID to every commit message, because the PR is already linked to the issue.
This creates a lot of unnecessary comments on the issue, so could you please leave them out next time?

app/components/confirm_modal_component.rb

app/components/confirm_modal_component/confirm_modal_component.html.haml

app/components/confirm_modal_component.rb

app/services/product_deleter.rb

app/reflexes/products_reflex.rb

app/webpacker/controllers/products_controller.js

app/views/admin/products_v3/_delete_modals.html.haml

chahmedejaz · 2023-12-13T18:56:37Z

you don't need to add the issue ID to every commit message, because the PR is already linked to the issue.
This creates a lot of unnecessary comments on the issue, so could you please leave them out next time?

Sure, David. I'll take care of this next time. :)
I just have the habit of linking commits to the issue as it helps to keep track of in which issue the commit was added.
From now I'll just add the ID in the commit without the hash. This wouldn't create a comment for the commit on the issue.
Does that work?

Edit: Had to rebase to resolve the conflict, and this recreated all of the commits. And the commits having issue references are again commented on the issue 😅 Sorry about that. I'll avoid the direct issue reference in the commit message in future PRs.

chahmedejaz · 2023-12-13T19:02:06Z

Thank you for your comments, David. Let me incorporate those and get back to you. Thanks. 😄

chahmedejaz · 2023-12-19T20:42:09Z

The test seems flaky as:
It's working as expected on my machine:

But getting failed on the GitHub workflow😅 :

sigmundpetersen · 2023-12-19T21:04:51Z

I re-ran the failed build.
One of the failures was flaky.
But the other seems persistent, could you look into it?

chahmedejaz · 2023-12-19T22:39:46Z

I re-ran the failed build. One of the failures was flaky. But the other seems persistent, could you look into it?

Those specs are working fine on my machine, strange 😅

I looked into the issue further, seems like a file is missing in the test env which should be created by the code itself. Can you please rerun the build, maybe it gets created. Thank you.

chahmedejaz · 2023-12-20T07:52:07Z

Thank you @sigmundpetersen for your help here. That's strange my changes are not even related 😢
Let me dig into it further.

sigmundpetersen · 2023-12-20T07:52:25Z

Ok, I think we need the perspective from a @openfoodfoundation/core-devs on this

# Conflicts: # app/views/admin/products_v3/components/_product_actions.html.haml

…ptions

…ntroller values

chahmedejaz · 2023-12-26T10:40:07Z

if you could change the container padding to 1.2rem

Hey @mariocarabotta - I've updated the modal container padding to 1.2rem. Please review and let me know how it looks. Thanks.

mariocarabotta · 2023-12-27T01:55:18Z

if you could change the container padding to 1.2rem

Hey @mariocarabotta - I've updated the modal container padding to 1.2rem. Please review and let me know how it looks. Thanks.

perfect! all good from my side - great job

mariocarabotta · 2023-12-28T05:11:41Z

created a separate issue for dealing with the above scenario mentioned by Maikel.

Don't reload whole table when table menu actions are performed

dacook

We're getting there! I have a couple of suggestions for simplifying some logic, and some questions.

app/views/admin/products_v3/_table.html.haml

app/reflexes/products_reflex.rb

app/views/admin/products_v3/_delete_modal.html.haml

app/components/help_modal_component/help_modal_component.scss

app/webpacker/controllers/product_actions_controller.js

spec/system/admin/products_v3/products_spec.rb

app/reflexes/products_reflex.rb

dacook · 2023-12-28T22:50:15Z

I've reviewed Maikel's comments and I think we're all good for testing now! ⏩

mkllnk

Great improvements, thank you.

The spec can still be improved but that shouldn't block the whole pull request. We can work on that separately.

mkllnk · 2024-01-02T21:38:02Z

spec/system/admin/products_v3/products_spec.rb

+  # Wait for an element with the given CSS selector and class to be present
+  def wait_for_class(selector, class_name)
+    max_wait_time = Capybara.default_max_wait_time
+    Timeout.timeout(max_wait_time) do
+      sleep(0.1) until page.has_css?(selector, class: class_name, visible: false)
+    end
+  end


This shouldn't be necessary because has_css? waits already. It should be the same as:

Suggested change

# Wait for an element with the given CSS selector and class to be present

def wait_for_class(selector, class_name)

max_wait_time = Capybara.default_max_wait_time

Timeout.timeout(max_wait_time) do

sleep(0.1) until page.has_css?(selector, class: class_name, visible: false)

end

end

# Wait for an element with the given CSS selector and class to be present

def wait_for_class(selector, class_name)

page.has_css?(selector, class: class_name, visible: false)

end

But I guess that it didn't work? I wonder why...

It works when used with visible visible: false as suggested.

page.has_css?(selector, class: class_name, visible: false) returns a boolean and I thought it would make more sense to use it as a boolean. 😅 i.e. if the loader is still not hidden, then make sure to wait until it's hidden.

If it is already hidden as the result of the wait in has_css? then the implemented sleep(0.1) won't be triggered.

If we still wanna commit the suggestion, then please let me know. No worries, we can. :)

This PR is ready for testing already. I don't want to delay it. We can optimise specs another time.

RachL · 2024-01-08T12:17:17Z

I haven't managed to reproduce a failed deletion action. Other than that all 13 other scenario do work, thank so much @chahmedejaz !

@mariocarabotta as I'm merging you should have the action available next time you stage something or if you stage master/main branch.

chahmedejaz · 2024-01-08T13:31:15Z

I haven't managed to reproduce a failed deletion action.

Thanks for validating this, @RachL
Actually this scenario is very rare to reproduce in a production environment.
It will happen when the delete database transaction fails. It's unlikely to happen I believe.
However for the sake of validation, a dev can forcefully reproduce this scenario locally.
Please let me know if want me to share the loom for this. Thanks 🙂

chahmedejaz force-pushed the task/11068-delete-product-or-variant branch 4 times, most recently from 86446a4 to 8541bb1 Compare December 3, 2023 18:29

dacook requested changes Dec 13, 2023

View reviewed changes

chahmedejaz force-pushed the task/11068-delete-product-or-variant branch from 2c37d31 to b1f338d Compare December 13, 2023 21:21

sigmundpetersen assigned chahmedejaz Dec 14, 2023

chahmedejaz added 13 commits December 20, 2023 12:54

openfoodfoundation#11068, add delete dropdown option

39de2f7

# Conflicts: # app/views/admin/products_v3/components/_product_actions.html.haml

openfoodfoundation#11068, add delete option for product variant

0ff1067

openfoodfoundation#11068, update ConfirmModalComponent to take more o…

5d490de

…ptions

openfoodfoundation#11068, add delete modals

4f76478

openfoodfoundation#11068, add ProductDeleter

bf0943e

openfoodfoundation#11068, add product_actions_controller

2f0965a

11068, revert controller changes

d7f2cbf

openfoodfoundation#11068, resolve conflicts

d1224ea

openfoodfoundation#11068: update confirm_modal_component to accept co…

0efd78b

…ntroller values

openfoodfoundation#11068: add product_v3 delete_modals partial

c202324

11068, apply delete_modals partial

5d90993

openfoodfoundation#11068, update delete links for uniq modal ids

eca83ee

openfoodfoundation#11068: add delete variant and product reflexes

05e7674

mariocarabotta mentioned this pull request Dec 26, 2023

send-shipment-email-optionally #11762

Merged

4 tasks

11068: update modal padding

86c2397

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Dec 27, 2023

mariocarabotta mentioned this pull request Dec 28, 2023

[BUU] Don't reload whole table when cloning products #11987

Closed

dacook reviewed Dec 28, 2023

View reviewed changes

chahmedejaz added 7 commits December 28, 2023 17:06

11068: update variant_scope method to inline

0fd868b

11068: add black-text css class

047e635

11068: add confirm_actions to close the modal

813ebd9

11068: replace p tag with a tag

548ffc0

11068: add sleep of 0.1s

f194b03

11068: update setModalDataset logic to generic logic

17bd3bb

11068: fix specs

1d6a089

chahmedejaz requested a review from dacook December 28, 2023 17:12

11068: add responsiveness to delete modal actions

c2693d1

drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 28, 2023

dacook approved these changes Dec 28, 2023

View reviewed changes

drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 28, 2023

mkllnk approved these changes Jan 2, 2024

View reviewed changes

RachL added the pr-staged-fr staging.coopcircuits.fr label Jan 8, 2024

RachL self-assigned this Jan 8, 2024

RachL merged commit bd0a296 into openfoodfoundation:master Jan 8, 2024
54 checks passed

RachL removed the pr-staged-fr staging.coopcircuits.fr label Jan 8, 2024

dacook mentioned this pull request Apr 18, 2024

[BUU] Don't reload whole table when deleting products or variants #12398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Add option to delete a product or variant #11846

[BUU] Add option to delete a product or variant #11846

chahmedejaz commented Nov 22, 2023 •

edited by RachL

Loading

dacook commented Dec 5, 2023

chahmedejaz commented Dec 5, 2023

dacook commented Dec 5, 2023

dacook left a comment

chahmedejaz commented Dec 13, 2023 •

edited

Loading

chahmedejaz commented Dec 13, 2023

chahmedejaz commented Dec 19, 2023

sigmundpetersen commented Dec 19, 2023

chahmedejaz commented Dec 19, 2023 •

edited

Loading

chahmedejaz commented Dec 20, 2023

sigmundpetersen commented Dec 20, 2023

chahmedejaz commented Dec 26, 2023

mariocarabotta commented Dec 27, 2023

mariocarabotta commented Dec 28, 2023

dacook left a comment

dacook commented Dec 28, 2023

mkllnk left a comment

mkllnk Jan 2, 2024

chahmedejaz Jan 3, 2024

mkllnk Jan 3, 2024

RachL commented Jan 8, 2024

chahmedejaz commented Jan 8, 2024

[BUU] Add option to delete a product or variant #11846

[BUU] Add option to delete a product or variant #11846

Conversation

chahmedejaz commented Nov 22, 2023 • edited by RachL Loading

What? Why?

What should we test?

dacook commented Dec 5, 2023

chahmedejaz commented Dec 5, 2023

dacook commented Dec 5, 2023

dacook left a comment

Choose a reason for hiding this comment

chahmedejaz commented Dec 13, 2023 • edited Loading

chahmedejaz commented Dec 13, 2023

chahmedejaz commented Dec 19, 2023

sigmundpetersen commented Dec 19, 2023

chahmedejaz commented Dec 19, 2023 • edited Loading

chahmedejaz commented Dec 20, 2023

sigmundpetersen commented Dec 20, 2023

chahmedejaz commented Dec 26, 2023

mariocarabotta commented Dec 27, 2023

mariocarabotta commented Dec 28, 2023

dacook left a comment

Choose a reason for hiding this comment

dacook commented Dec 28, 2023

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jan 2, 2024

Choose a reason for hiding this comment

chahmedejaz Jan 3, 2024

Choose a reason for hiding this comment

mkllnk Jan 3, 2024

Choose a reason for hiding this comment

RachL commented Jan 8, 2024

chahmedejaz commented Jan 8, 2024

chahmedejaz commented Nov 22, 2023 •

edited by RachL

Loading

chahmedejaz commented Dec 13, 2023 •

edited

Loading

chahmedejaz commented Dec 19, 2023 •

edited

Loading