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 index starts from 1 #7785

Merged
merged 1 commit into from May 22, 2017

Conversation

Projects
None yet
3 participants
@sairam
Contributor

sairam commented Jan 15, 2017

(Identified in older version of spree . Fix made to solidus fork at solidusio/solidus@ad40374 )
Javascript is sending indices starting from 0.

acts_as_list has top_of_list defaulted as 1 - Source: https://github.com/swanandp/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L38

Steps to reproduce the Issue:

  1. Go to images under product.
  2. Take any element after the 1st one to the top.
  3. Edit the one at the top (that was just dragged)
  4. Coming back to the index page shows it in the 2nd position.

What is happening in the frontend?

  1. Javascript sends the indices starting 0.
  2. The update_positions method in resource_controller just sets the positions as they were sent (with more stuff going on)
  3. When element in the 0th position is modified. acts_as_list figures 1 is the top of the list and sets the value to 1 along with setting anything which is already in position 1 to 0.

This fix will not modify existing positions, but will only fix those resources for new updates.

Another solution could be to set the configuration for all models which use acts_as_list to top_of_list: 0 which could be an alternative solution which fixes this issue from re-occurring in the first place.

acts_as_list index starts from 1
(Identified in older version of spree . Fix made to solidus fork at solidusio/solidus@ad40374 )
Javascript is sending indices starting from 0. 

acts_as_list has top_of_list defaulted as 1 - Source: https://github.com/swanandp/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L38

Steps to reproduce the Issue: 
1. Go to images under product. 
2. Take any element after the 1st one to the top. 
3. Edit the one at the top (that was just dragged)
4. Coming back to the index page shows it in the 2nd position. 

What is happening in the frontend?
1. Javascript sends the indices starting 0. 
2. The update_positions method in resource_controller just sets the positions as they were sent (with more stuff going on)
3. When element in the 0th position is modified. acts_as_list figures 1 is the top of the list and sets the value to 1 along with setting anything which is already in position 1 to 0. 

This fix will not modify existing positions, but will only fix those resources for new updates.

Another solution could be to set the configuration for all models which use acts_as_list to top_of_list: 0 which could be an alternative solution which fixes this issue from re-occurring in the first place.
@krzysiek1507

This comment has been minimized.

Show comment
Hide comment
@krzysiek1507

krzysiek1507 Jan 15, 2017

Contributor

I'm not sure if we should send n+1 or start from 0 in models

Contributor

krzysiek1507 commented Jan 15, 2017

I'm not sure if we should send n+1 or start from 0 in models

@sairam

This comment has been minimized.

Show comment
Hide comment
@sairam

sairam Jan 16, 2017

Contributor

In order to fix things on demand, I feel the n+1 model is the better approach by respecting the defaults from the backend.

Contributor

sairam commented Jan 16, 2017

In order to fix things on demand, I feel the n+1 model is the better approach by respecting the defaults from the backend.

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

@damianlegawiec damianlegawiec merged commit 8df6b64 into spree:master May 22, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment