Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #8822 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[PATCH] acts_as_list remove_from_list method then fails in_list? test.

Reported by: mikel Assigned to: core
Priority: normal Milestone: 2.x
Component: Plugins Version: edge
Severity: normal Keywords: tiny patch ActiveRecord acts_as_list in_list? remove_from_list
Cc:

Description

Bug in acts_as_list remove_from_list

The error found:

Patch to fix a two problems with acts in list:

  • Removing an item from the list does not remove it from the list and returns true to in_list?
  • Removing an item from the list and then destroying the item shifts all lower items up two positions, creating duplicates

Console example of the first error: 

>> foo = Foo.create()
=> #<Foo:0x6979ab8 SNIP>
>> foo.in_list?
=> true
>> foo.remove_from_list
=> 0
>> foo.in_list?
=> true
>> foo.reload
=> #<Foo:0x6979ab8 SNIP>
>> foo.in_list?
=> true

For the demonstration of the second error, see the failing tests.

Handling

1. in_list? checks for nil in the position column. Handling is to set position to nil when you remove an instance from the list.

2. Also, only remove from list if the item is in list, wrapped this in an if in_list?.

3. This then breaks destroy as you can't nil a frozen hash. So I moved remove_from_list from after_destroy to before_destroy.

Tests

Three tests showing this are attached with the source code to patch it (5 lines in acts/list.rb)

Attachments

acts_as_list_remove_from_list_fix.diff (2.7 kB) - added by mikel on 10/09/07 02:50:52.
Patch to STABLE only. Fixed issues with the stable code moving underneath my last patch. Now applies cleanly and tests pass. (will upload a separate file for trunk on the plugins)
2.0-PLUGIN_acts_as_list_remove_from_list_fix.diff (2.4 kB) - added by mikel on 10/09/07 04:56:58.
Same patch for the 2.0 vendor/plugins/acts_as_list
1.2-STABLE_acts_as_list_remove_from_list_fix.diff (2.9 kB) - added by mikel on 10/09/07 05:12:22.
Fixed tests to match recent changes in code for stable branch

Change History

07/01/07 03:05:47 changed by mikel

  • version changed from 1.2.3 to edge.

The above code is done from the trunk, not stable. Changed correct version in ticket.

07/02/07 12:30:22 changed by mikel

  • keywords changed from acts_as_list in_list? remove_from_list activerecord to tiny patch ActiveRecord acts_as_list in_list? remove_from_list.

07/08/07 03:40:42 changed by mikel

  • keywords changed from tiny patch ActiveRecord acts_as_list in_list? remove_from_list to tiny verified patch ActiveRecord acts_as_list in_list? remove_from_list.

Have double checked all tests to be valid so this is verified.

07/19/07 22:59:26 changed by mpalmer

  • keywords changed from tiny verified patch ActiveRecord acts_as_list in_list? remove_from_list to tiny patch ActiveRecord acts_as_list in_list? remove_from_list.

mikel,

Just for your info, the 'verified' tag is for tickets that have had three people review the patch and validate that it's good to go. See http://wiki.rubyonrails.org/rails/pages/PatchRequirements for more info.

09/11/07 15:07:40 changed by josh

  • component changed from ActiveRecord to Plugins.

(follow-up: ↓ 8 ) 10/08/07 02:03:41 changed by bitsweat

The tests fail for me on 1-2-stable branch 

  1) Failure:
test_remove_before_destroy_does_not_shift_lower_items_twice(ListTest) [./test/mixin_test.rb:236]:
<#<ListMixin:0x327daa0
 @attributes=
  {"updated_at"=>nil,
   "type"=>"ListMixin",
   "lft"=>nil,
   "id"=>"1007",
   "root_id"=>nil,
   "rgt"=>nil,
   "pos"=>"1",
   "parent_id"=>"5",
   "created_at"=>nil}>> expected but was
<[#<ListMixin:0x3279a90
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1007",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"1",
    "parent_id"=>"5",
    "created_at"=>nil}>,
 #<ListMixin:0x327970c
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1008",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"2",
    "parent_id"=>"5",
    "created_at"=>nil}>,
 #<ListMixin:0x3278e9c
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1009",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"3",
    "parent_id"=>"5",
    "created_at"=>nil}>,
 #<ListMixin:0x3278aa0
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1010",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"4",
    "parent_id"=>"5",
    "created_at"=>nil}>]>.

  2) Failure:
test_remove_from_list_should_set_position_to_nil(ListTest) [./test/mixin_test.rb:221]:
<#<ListMixin:0x32532c8
 @attributes=
  {"updated_at"=>nil,
   "type"=>"ListMixin",
   "lft"=>nil,
   "id"=>"1007",
   "root_id"=>nil,
   "rgt"=>nil,
   "pos"=>"1",
   "parent_id"=>"5",
   "created_at"=>nil}>> expected but was
<[#<ListMixin:0x3251590
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1007",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"1",
    "parent_id"=>"5",
    "created_at"=>nil}>,
 #<ListMixin:0x3251270
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1008",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"2",
    "parent_id"=>"5",
    "created_at"=>nil}>,
 #<ListMixin:0x3250e9c
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1009",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"3",
    "parent_id"=>"5",
    "created_at"=>nil}>,
 #<ListMixin:0x3250c1c
  @attributes=
   {"updated_at"=>nil,
    "type"=>"ListMixin",
    "lft"=>nil,
    "id"=>"1010",
    "root_id"=>nil,
    "rgt"=>nil,
    "pos"=>"4",
    "parent_id"=>"5",
    "created_at"=>nil}>]>.

10/08/07 02:04:57 changed by bitsweat

Could you patch against /plugins/acts_as_list for compatibility with 2.0 also?

10/09/07 02:50:52 changed by mikel

  • attachment acts_as_list_remove_from_list_fix.diff added.

Patch to STABLE only. Fixed issues with the stable code moving underneath my last patch. Now applies cleanly and tests pass. (will upload a separate file for trunk on the plugins)

(in reply to: ↑ 6 ) 10/09/07 02:52:13 changed by mikel

I fixed the code by just reapplying it to stable. The patch was 3 months old and the mixins file had changed.

Now works on 1-2-stable. Will upload a plugins patch.

Mikel

Replying to bitsweat:

The tests fail for me on 1-2-stable branch {{{ 1) Failure: test_remove_before_destroy_does_not_shift_lower_items_twice(ListTest) ./test/mixin_test.rb:236: <#<ListMixin:0x327daa0 @attributes=

10/09/07 04:56:58 changed by mikel

  • attachment 2.0-PLUGIN_acts_as_list_remove_from_list_fix.diff added.

Same patch for the 2.0 vendor/plugins/acts_as_list

10/09/07 05:12:22 changed by mikel

  • attachment 1.2-STABLE_acts_as_list_remove_from_list_fix.diff added.

Fixed tests to match recent changes in code for stable branch

10/09/07 05:16:46 changed by nzkoz

  • status changed from new to closed.
  • resolution set to fixed.

(In [7815]) Make sure acts_as_list's remove_from_list and in_list? play nicely with one another. Closes #8822 [mikel]

10/09/07 05:31:22 changed by nzkoz

(In [7816]) Make sure acts_as_list's remove_from_list and in_list? play nicely with one another. References #8822 [mikel]