[BUU] Change name of my products 🚧 #11208

dacook · 2023-07-13T07:13:38Z

What? Why?

Partially implements [BUU] Change name, SKU and price of my products and variants #11059

Adding a new bulk update form for product names, submitting using Reflex.
You can edit the name of any product (but we don't yet highlight modified/unsaved fields)
On saving, any modified products will attempt to update
Any errors are captured and some information is displayed to the user (more to be added in next PR).
On successful save, the table reloads to show saved fields (but there's no success message yet).

Editing:

Errors:

What should we test?

No testing required, this is a work in progress behind feature toggle.

Release notes

Changelog Category: Feature toggle

spec/reflexes/products_reflex_spec.rb

dacook · 2023-08-18T07:43:45Z

Oops, I missed a broken spec. JB I think it would still be worth your review if you have the chance. I'll next look at it on Tuesday.

jibees

I missed a broken spec.

I suppose this was a flaky one?

BTW, real nice job!

Quick comments:

Should we hide form-actions toolbar if no modification are currently made?
I tried to modify a product name, but I received an error. Even if I don't change any name, and click on "Save changes" button, I received an error ("

#<ActiveModel::Error attribute=base, type=Image attachment is not a valid image., options={}>

I suppose this is related to an old product on my page?

jibees · 2023-08-18T09:00:24Z

app/reflexes/products_reflex.rb

+    ).broadcast
+    morph :nothing
+
+    # dunno why this doesn't work.


It's not clear to me as well.

jibees · 2023-08-18T09:15:16Z

app/webpacker/css/admin/products_v3.scss

+
+    // "Naked" inputs. Row hover helps reveal them.
+    input {
+      border-color: transparent;


No need to change background color as well?

Good question. Currently, the input background and the row background are white.
When we have row:hover, background-color: $light-grey; and we rely on the input background-color: white.

So technically, the code doesn't ensure the inputs are naked (eg if one of the background colours change in the future), and I think we could add more rules to ensure that. But I don't think we should add more style rules until we need them.

app/webpacker/css/admin_v3/components/buttons.scss

jibees · 2023-08-18T09:48:17Z

.rubocop_todo.yml

@@ -220,11 +220,13 @@ Metrics/ClassLength:
    - 'app/models/spree/user.rb'
    - 'app/models/spree/variant.rb'
    - 'app/models/spree/zone.rb'
+    - 'app/reflexes/products_reflex.rb'


seems legit 👌

(For now at least,) we use one big standard Rails form, and ModelSet to update each record. Submitting with Reflex allows us to manage the loading state along with the rest of the page (although I would rather use the built in HTTP POST standard). Aria-label makes it a bit easier for testing (and accessibility software of course!). Technically it should have been aria-labelledby="id_of_column_header" but that would have resulted in more HTML and processing, which seemed silly. Best viewed with whitespace ignored.

Perhaps this should be tested in the system spec too ("I can rename a product and still see it after saving"). But I'd like to find the compromise to avoid bulking up system specs too much. I think it's covered well enough by the reflex spec?

Before, it would abort after the first invalid record, and it doesn't tell you about the others. This way you find out about all at once. This affects the existing Bulk Edit Products screen, and can result in longer error messages than before. But I would argue that's a good thing. I think this is technically optional for BUU at this point, but a helpful improvement.

But it's not perfect. Can we use a pseudo element instead?

Inputs add extra padding, so we add the same padding to the header. Using an opt-in class, because I think we won't want this on all columns.

As much as I hate to add to this list, this is still a work in progress so it's not worth refactoring at this point.

dacook · 2023-08-22T01:53:50Z

Thanks @jibees,

Should we hide form-actions toolbar if no modification are currently made?

For sure. I plan to do this when implementing the modified/"dirty" field state.

I tried to modify a product name, but I received an error.

I suppose this is related to an old product on my page?

Yes I would guess so. It might be a good time to refresh your dev database.

mkllnk

I have a couple of suggestions but this is work in progress and it can all be changed later.

mkllnk · 2023-08-23T04:48:44Z

app/reflexes/products_reflex.rb

+    # TO Consider: We could actually rearrange the form to suit that format more directly. eg:
+    #   '[products][0][id]' (hidden field)
+    #   '[products][0][name]'


I guess that this is necessary when you want to support creating several new products in the same form. Then you can't use the id as index. But I find this approach of stuffing all kind of data into one action unnecessarily complex. Doing lots of stuff at once has the challenge of lots of things potentially failing and providing lots of error feedback while also adding a lot of logic regarding different actions to one thing. In this case the ModelSet encapsulates it quite well but it's still complicated.

Good points. At this stage, we want to provide that behaviour (updating multiple at once) so I think it makes sense to use the one form, for now. So far we haven't hit any issues with this method, so I'd avoid complicating it. For now..

What about new products or deleting products? Will there be a separate form or UX?

I assume, new products will continue to be with a specific form, and deleting would be one-by-one.

But if we needed to do it in bulk here, then we could.. but like you say it makes things complicated!

app/services/sets/product_set.rb

Rails is clever enough to not query the database without ids Co-authored-by: Maikel <maikel@email.org.au>

dacook

Great, thank you!

app/services/sets/product_set.rb

drummer83 · 2023-08-24T07:48:57Z

Hi @dacook,
This one ended up in 'Test Ready' but it's stating 'no testing required'.
Please proceed with this one as needed or let us know if testing is required.
Thanks!
BTW: Looking great! 😉

filipefurtad0 · 2023-08-24T20:19:41Z

Ok, I've staged the PR just as a quick check whether it staged with no issues, and made some bulk product edits, with the feature toggle on and off.

Nothing unusual, so merging not to block this work 👍

dacook · 2023-08-24T23:45:19Z

Thanks both! I think I wasn't aware of the pr-no-test label when I created this PR, but will try to remember in the future to help streamline things.

As discussed in openfoodfoundation#11208 (comment)

dacook force-pushed the buu-editing-11059 branch from b36b21b to f205f0a Compare July 28, 2023 05:46

dacook changed the base branch from master to backoffice-ui-uplift July 28, 2023 05:48

dacook force-pushed the buu-editing-11059 branch from f205f0a to 5775aff Compare July 28, 2023 05:57

mkllnk reviewed Jul 28, 2023

View reviewed changes

spec/reflexes/products_reflex_spec.rb Show resolved Hide resolved

dacook force-pushed the backoffice-ui-uplift branch from f6a64f0 to d6ccd95 Compare August 8, 2023 05:40

dacook force-pushed the buu-editing-11059 branch 2 times, most recently from 862114c to 91f0c1f Compare August 9, 2023 03:34

dacook force-pushed the backoffice-ui-uplift branch 2 times, most recently from 868b648 to 73bfe63 Compare August 10, 2023 03:47

jibees force-pushed the backoffice-ui-uplift branch 5 times, most recently from 90fdda1 to e9640d7 Compare August 14, 2023 12:42

dacook force-pushed the buu-editing-11059 branch from 91f0c1f to 052b7c7 Compare August 18, 2023 05:15

dacook changed the base branch from backoffice-ui-uplift to master August 18, 2023 05:15

dacook force-pushed the buu-editing-11059 branch 2 times, most recently from bcd809a to b3a7e14 Compare August 18, 2023 06:36

dacook changed the title ~~Buu editing 11059~~ [BUU] Change name of my products 🚧 Aug 18, 2023

dacook marked this pull request as ready for review August 18, 2023 06:48

dacook requested a review from jibees August 18, 2023 06:49

dacook force-pushed the buu-editing-11059 branch from b3a7e14 to 32414d4 Compare August 18, 2023 06:54

jibees approved these changes Aug 18, 2023

View reviewed changes

sigmundpetersen assigned dacook Aug 21, 2023

dacook added 5 commits August 22, 2023 11:40

Capybara: enable_aria_label

a98242e

Retain the order of products in the collection

6ffe1ec

Retain order when saving.

71c3658

Perhaps this should be tested in the system spec too ("I can rename a product and still see it after saving"). But I'd like to find the compromise to avoid bulking up system specs too much. I think it's covered well enough by the reflex spec?

dacook added 9 commits August 22, 2023 11:40

[wip] Left line for row hover

be24247

But it's not perfect. Can we use a pseudo element instead?

[fixup] Left line for row hover

2b09ec7

Remove unused style

8440c44

Align row headers with input contents

37d1113

Inputs add extra padding, so we add the same padding to the header. Using an opt-in class, because I think we won't want this on all columns.

Copy form styles to v3

3cc5d7b

Update link colours to match new design

7bf7944

Enable 'medium' style for reset buttons

fccde96

Style form actions area

b0e77df

Ignore long classes for now

2edf504

As much as I hate to add to this list, this is still a work in progress so it's not worth refactoring at this point.

dacook force-pushed the buu-editing-11059 branch from 32414d4 to 2edf504 Compare August 22, 2023 01:51

mkllnk approved these changes Aug 23, 2023

View reviewed changes

Apply suggestions from code review

a8d37d0

Rails is clever enough to not query the database without ids Co-authored-by: Maikel <maikel@email.org.au>

dacook commented Aug 23, 2023

View reviewed changes

app/services/sets/product_set.rb Outdated Show resolved Hide resolved

drummer83 added the feedback-needed label Aug 24, 2023

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 24, 2023

filipefurtad0 merged commit cc3b7c6 into openfoodfoundation:master Aug 24, 2023
54 checks passed

filipefurtad0 removed pr-staged-uk staging.openfoodnetwork.org.uk feedback-needed labels Aug 24, 2023

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Aug 29, 2023

dacook mentioned this pull request Aug 31, 2023

[BUU] Editing - part 2 (product and variant text fields) 🚧 #11468

Merged

3 tasks

dacook added a commit to dacook/openfoodnetwork that referenced this pull request Sep 6, 2023

Enforce block style for rspec .change

0018cac

As discussed in openfoodfoundation#11208 (comment)

dacook added a commit to dacook/openfoodnetwork that referenced this pull request Sep 6, 2023

Enforce block style for rspec .change

4a4bf92

As discussed in openfoodfoundation#11208 (comment)

dacook added a commit to dacook/openfoodnetwork that referenced this pull request Feb 23, 2024

Enforce block style for rspec .change

eaacaf4

As discussed in openfoodfoundation#11208 (comment)

dacook mentioned this pull request Feb 23, 2024

Add RSpec cops #11500

Merged

8 tasks

fleblond07 pushed a commit to fleblond07/openfoodnetwork that referenced this pull request Mar 13, 2024

Enforce block style for rspec .change

f0a0de1

As discussed in openfoodfoundation#11208 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Change name of my products 🚧 #11208

[BUU] Change name of my products 🚧 #11208

dacook commented Jul 13, 2023 •

edited

dacook commented Aug 18, 2023

jibees left a comment

jibees Aug 18, 2023

jibees Aug 18, 2023

dacook Aug 22, 2023

jibees Aug 18, 2023

dacook commented Aug 22, 2023 •

edited

mkllnk left a comment

mkllnk Aug 23, 2023

dacook Aug 23, 2023

mkllnk Aug 24, 2023

dacook Aug 24, 2023

dacook left a comment

drummer83 commented Aug 24, 2023

filipefurtad0 commented Aug 24, 2023

dacook commented Aug 24, 2023

[BUU] Change name of my products 🚧 #11208

[BUU] Change name of my products 🚧 #11208

Conversation

dacook commented Jul 13, 2023 • edited

What? Why?

What should we test?

Release notes

dacook commented Aug 18, 2023

jibees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook commented Aug 22, 2023 • edited

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Aug 24, 2023

filipefurtad0 commented Aug 24, 2023

dacook commented Aug 24, 2023

dacook commented Jul 13, 2023 •

edited

dacook commented Aug 22, 2023 •

edited