-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[BUU] Add option to delete a product or variant #11846
[BUU] Add option to delete a product or variant #11846
Conversation
86446a4
to
8541bb1
Compare
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? |
I'm almost done with it. P.S. Sorry for taking this long, actually got sick in between and couldn't work for a good chunk.😅 |
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 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/confirm_modal_component.html.haml
Outdated
Show resolved
Hide resolved
Sure, David. I'll take care of this next time. :) 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. |
Thank you for your comments, David. Let me incorporate those and get back to you. Thanks. 😄 |
2c37d31
to
b1f338d
Compare
I re-ran the failed build. |
Thank you @sigmundpetersen for your help here. That's strange my changes are not even related 😢 |
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
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 |
created a separate issue for dealing with the above scenario mentioned by Maikel. Don't reload whole table when table menu actions are performed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there! I have a couple of suggestions for simplifying some logic, and some questions.
I've reviewed Maikel's comments and I think we're all good for testing now! ⏩ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary because has_css?
waits already. It should be the same as:
# 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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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 implementedsleep(0.1)
won't be triggered.
If we still wanna commit the suggestion, then please let me know. No worries, we can. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is ready for testing already. I don't want to delay it. We can optimise specs another time.
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. |
Thanks for validating this, @RachL |
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?
Change log Category: