Unified Handling of Option Values and Product Properties List

[hefan]

1. Add a default empty entry if there is no entry
2. "Add new" dynamically produces new items below all other items
3. do never add a sort option before save
4. add a remove button for non persistent fields
5. deprecate remove_button helper

see also #3561 and #3581

The PR removes the mix of removing(nonpersistent) and deleting(persistent) records.
It also unifies the way new records are added (the are always marked as new and don't copy ids of other table rows)

One Catch is remaining (as already before this PR):
If you delete the first empty default entry you cannot add new entries. You have to reload the Page to see a new empty entry

possible solutions:

1. do never add remove buttons for non persistent fields. You voted against that in adjust option value handling to work like product properties handling #3561. With Commit a08b022 this would be done, the following commits handle the remove functionality.

2. Do somehow rewrite the JS $('.spree_add_fields').click(function() {}) here https://github.com/solidusio/solidus/blob/master/backend/app/assets/javascripts/spree/backend/admin.js to not need a template or to get the template for a new row from another place.
   Any Suggestions for that?

[kennyadsl]

Thanks @hefan!

I left a couple of questions and I have other two things:

1. At the moment, even if the row doesn't have the handle, it's included in the draggable items list, and I think we can easily remove it by adding the draggable: option to Sortable, the plugin that takes care of this. In this way, the new non-persisted row will be excluded.
2. Can we squash some commits? I feel like some of them are useless and can be grouped together, maybe with a better description.

Thanks again!

[kennyadsl]

Why is this change needed? Shouldn't we be able to intercept the link using the previous version of the code?

[hefan]

before the nonpersistent icons didn't have a remove button. So here are 2 trash icons and we need to distinguish between remove and delete.

[kennyadsl]

Maybe persisted is more used? Just asking cause I never heard about persistent for this.

[hefan]

you are right

[hefan]

1. You mean by adding draggable to the Table classes? Does that suffice? Seems so.

2. i understood that small commits are preferred. So i tried to put it in a sequence somehow. There is code removed to get it to work without the remove option (which could be a way to handle this also) and then put back in. i would also prefer to do it all in one commit, because at the end you cannot use just parts of it.

Maybe i squash and rebase it all into one commit.

[hefan]

ok. added draggable, renamed spec methods and squashed/rebased.
Tell me if you need other commit groups. But its quite not clear how to split it into meaningful single commits.

[kennyadsl]

For draggable, I didn't mean that. I'm not sure what that class actually changes, why do you think it's needed?

The problem I'd like to solve is this one:

As you can see, dragging an element with the handle at the end of the table, when there's a non-persisted element, will make the non-persisted element move up in the table order. If you read the sortable plugin documentation, there's a draggable option that you can use when initializing the plugin to mark which elements are draggable and which aren't.

For commits, it's not true that you can't use just parts of this PR, for example, the deprecation can go in a separate commit and also the controller check that doesn't build the extra row if not empty. Isn't it?

[hefan]

1. Split Commits

I assume commits which would have to be reverted with the next commit are not wanted (the tests needs to be changed depending on sortable nonpersistent items or removable nonpersistent items). Also the system should not be let in a poorer state when only applying some commits (for example). So the only commits i see which would make a little bit of sense alone could be like you wrote:

a.) Controller Creation of new items only if no items present
b.) deprecation of helper function
c.) Always produce new items below the list
d.) remove the sort option in the list

Would you mind explaining a little bit what the point of splitting commits are here? Shouldn't this PR be added as a whole? Are non working partial commits also ok?

2. Sortable

I understood the problem but was not sure about the solution nor i know much about Sortable. I somehow thought that when adding draggable means that non sortable items are no more draggable also. This was wrong. But non draggable also does not mean they are not changed in position when dragging other items below.

To change the initialization of the Plugin we would need to mess with https://github.com/solidusio/solidus/blob/master/backend/vendor/assets/javascripts/solidus_admin/Sortable.js
or at least this
https://github.com/solidusio/solidus/blob/master/backend/app/assets/javascripts/spree/backend/components/sortable_table.js
right?
There is no draggable option as i understood from here.
https://api.jqueryui.com/sortable/
"By default, sortable items share draggable properties."

This could be a working solution for the problem
https://stackoverflow.com/questions/13885665/how-to-exclude-an-element-from-being-dragged-in-sortable-list

[kennyadsl]

This is the plugin used: https://github.com/SortableJS/Sortable

[kennyadsl]

Anyway, commits are needed for documentation. When a developer (even the core team reviewing the code) will look at commits, it's a great source of information. They should also contain the reason why that change happened. I suggest you to read this great article: https://mislav.net/2014/02/hidden-documentation/.

Anyway, it's pretty personal to balance how to break PRs code down into commits. It's very up to the developer submitting the patch. I use to walk in the shoes of other developers that, some months from now, will debug that code and git blame on a line seeing what changed and why. The smallest and indipendent that piece of information is, the easiest it will be for them to understand and quickly fix the code.

[hefan]

ok. thank you. i will read and do a commit remix tomorrow!

The sortable/draggable stuff is also an extra commit i assume. Couldn't it be also an extra PR? Because it is also an isolated improvement compared to the current code (for both lists)!

[hefan]

Now its regrouped in some isolated commits which each make sense. I hope its ok.

The draggable problem remains (just like before). i would like to tackle that in another Commit. Just altering the
https://github.com/solidusio/solidus/blob/master/backend/app/assets/javascripts/spree/backend/components/sortable_table.js
with a draggable option seemed to dangerous for me. There are many other Backend Tables which rely on these sortable tables and would need to add that element also.
Any Suggestions?

[kennyadsl]

I left some other questions + I think we can get rid of the last commit.

Thanks for taking the time to improve the PR!

[hefan]

Are the above assumptions/suggestions consensual?

[hefan]

i updated the PR accordingly to the suggestions.

[kennyadsl]

One last small thing and we are good to go for me, sorry if I didn't catch that before. And thanks again, this PR is much needed. 🙏

[hefan]

Yes you are right.
i assume that the user should not have the possibility to remove nor delete if he cannot destroy. right?

another thing came to my mind:
i guess if someone had previously overriden the _option_value_fields.html.erb he would rely on the old method of removing non persistent fields. Should this kind of change then be noted in a changelog or something?

[hefan]

ok, all done..

[kennyadsl]

Thanks @hefan!

[aldesantis]

Thanks @hefan! I just left a small comment about extracting the remove links into a helper.