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

Acts as List shuffling on zero item on Variants #7996

Closed
jkelleyj opened this Issue May 12, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@jkelleyj
Contributor

jkelleyj commented May 12, 2017

When reordering a list in spree backend, it uses a zero index as the position for items in the list. One example of this is on variants. This makes the list position indexes run from 0 to N-1.

If you later make ANY update to the item at the zero position (not just reordering), it is moved to 1 and the item at 1 is moved back to 0, swapping those two items.

The result is that we have items that inadvertently reorder.

Context

This issue impacts stores that rely on a specific ordering of varaints (or other reorderable object) since the first and second items in that list can reorder.

Expected Behavior

The list order should remain accurate for the first two items even if a property on either of them is touched.

Actual Behavior

Item 0 and item 1 are flipped.

Possible Fix

I think we have two options. One is to change the implementation of sortable in the JS to use a starting index of 1 instead of 0. The other is to update our use of acts_as_list to use the top_of_list property and set it to 0 instead of 1. Either approach could have backwards compatibility issues since lists are currently stored in both 1 first and 0 first sequences.

Here's a simple fix we could drop in the JS in backend/admin.js (just adding + 1 to the position)

$.each($('table.sortable tbody tr'), function(position, obj){
  reg = /spree_(\w+_?)+_(\d+)/;
  parts = reg.exec($(obj).prop('id'));
  if (parts) {
    positions['positions['+parts[2]+']'] = position + 1;
  }
});

A different solution which I like less is to update the ResourceController:

def update_positions
    ActiveRecord::Base.transaction do
      params[:positions].each do |id, index|
        model_class.find(id).set_list_position(index.to_i + 1)
      end
    end

    respond_to do |format|
      format.js { render text: 'Ok' }
    end
end

Steps to Reproduce

  1. Visit your variants page for a product and reorder the items. Note which item is now at top of the list
  2. Edit some attribute of the item that is now at the to of the list
  3. Observe in your variants list that this item has now moved to the second position

Your Environment

Spree 3.1

@jkelleyj

This comment has been minimized.

Show comment
Hide comment
@jkelleyj

jkelleyj May 12, 2017

Contributor

@damianlegawiec I'm curious for your take on the better approach to fix this when considering how to handle lists that could be in either a zero first or 1 first orientation now.

One thought is to go w/ 1 first (fix the JS) and then have a migration that does an update of all product variants where there is one in product's list that has a zero.

We could ignore the migration since some stores may have some assumption built into their store about the starting index (or a dependency on specific positions). In playing around with this, the JS fix will not successfully overwrite an existing 0 due to the way acts_as_list updates neighbors while updating via after_update :update_positions. As a result, only lists that are updated to not have a position 0 item will avoid this issue.

I'd say let's fix the JS not the acts_as_list inclusion. Then, unless you tell me there are no backwards compatibility issues with updating list positions in the migration, leave the migration to the individual stores. Any new products will behave correctly, but stores would need to update their positions to start at zero with a bit of sql like:
update spree_variants set position = position + 1 where product_id in (select product_id from spree_variants where position = 0);

Contributor

jkelleyj commented May 12, 2017

@damianlegawiec I'm curious for your take on the better approach to fix this when considering how to handle lists that could be in either a zero first or 1 first orientation now.

One thought is to go w/ 1 first (fix the JS) and then have a migration that does an update of all product variants where there is one in product's list that has a zero.

We could ignore the migration since some stores may have some assumption built into their store about the starting index (or a dependency on specific positions). In playing around with this, the JS fix will not successfully overwrite an existing 0 due to the way acts_as_list updates neighbors while updating via after_update :update_positions. As a result, only lists that are updated to not have a position 0 item will avoid this issue.

I'd say let's fix the JS not the acts_as_list inclusion. Then, unless you tell me there are no backwards compatibility issues with updating list positions in the migration, leave the migration to the individual stores. Any new products will behave correctly, but stores would need to update their positions to start at zero with a bit of sql like:
update spree_variants set position = position + 1 where product_id in (select product_id from spree_variants where position = 0);

@damianlegawiec damianlegawiec added this to the v3.3.x milestone May 21, 2017

@damianlegawiec damianlegawiec added the Bug label May 21, 2017

@damianlegawiec damianlegawiec self-assigned this May 21, 2017

@damianlegawiec

This comment has been minimized.

Show comment
Hide comment
@damianlegawiec

damianlegawiec May 21, 2017

Member

@jkelleyj fixing it on JS side + the migration seems to be the best approach! Can you submit a PR with those changes?

Member

damianlegawiec commented May 21, 2017

@jkelleyj fixing it on JS side + the migration seems to be the best approach! Can you submit a PR with those changes?

@krzysiek1507

This comment has been minimized.

Show comment
Hide comment
@krzysiek1507

krzysiek1507 May 21, 2017

Contributor

There is PR #7785 with JS fix.

Contributor

krzysiek1507 commented May 21, 2017

There is PR #7785 with JS fix.

@jkelleyj

This comment has been minimized.

Show comment
Hide comment
@jkelleyj

jkelleyj May 22, 2017

Contributor

That PR looks good to me. That's the same tweak I made in my 3.1 installation (with a little whitespace around the +)

Contributor

jkelleyj commented May 22, 2017

That PR looks good to me. That's the same tweak I made in my 3.1 installation (with a little whitespace around the +)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment