Remove branch assume_default_position? in add_new_at

[eeeichan]

The following is the case of a premise, the movement of position will be incorrect.

Prerequisites

top_of_list	add_new_at	default_position
0	:top	1

e.g. Add a record with position 1	id	name	position
1	aaaaa	0

↓

id	name	position
1	aaaaa	1
2	bbbbb	0

Reason

When the default position is 1, the value of assume_default_position? is true. Then, increment_positions_on_all_items turns a record in position 0 into position 1. Finally, acts_as_list_top (0) is assigned to the record in position 1.

          when :top
            if assume_default_position?
              increment_positions_on_all_items
              self[position_column] = acts_as_list_top

https://github.com/brendon/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L234-L237

Solution

We do not consider the assignments of increment_positions_on_all_items and acts_as_list_top to be necessary. Only increment_positions_on_lower_items works fine.

[eeeichan]

This pull request is a suggestion and we would be happy to hear your input.

[brendon]

Hi @eeeichan, that's an interesting setup. Just wondering why you'd set the top of the list as 0 and then set the default position at 1 not 0?

[eeeichan]

Hi @brendon, Thanks for taking a look at this PR :)

Sorry, I don't know, I recently joined a project that uses this acts_as_list. We think it's a configuration leak...

Or the default value may have been changed in the process of updating by position, because in the dev environment the default value for similar records is 0.

• Where can I set default_position? Is it possible to set it up outside of migrate? And is it possible to set defalut: 0 (e.g. t.integer :position, null: false, default: 0 ) ? https://github.com/brendon/acts_as_list#adding-acts_as_list-to-an-existing-model

[brendon]

It is possible to set the default on the database but I'd recommend just setting it in acts_as_list. The readme should show how to do that as an option. You just pass it to acts_as_list. It could well be that the code won't handle that strange scenario well, but it probably never has.

[eeeichan]

I understand. I am sure that the unexpected process is working due to a problem with the configuration values. But I don't think the assume_default_position? branch is necessary, may I ask why you are intentionally assigning acts_as_list_top after doing the increment_positions_on_all_items ? I think it's unnecessary to branch out.

By the way, the problem with the position being 1 was completely a problem with our environment. We apologize for the inconvenience.

[brendon]

That's not a problem :) I'm always happy to answer questions.

We need to check if we're to put the current item at the default position (in this case at the top of the list):

def assume_default_position?
  not_in_list? ||
  persisted? && internal_scope_changed? && !position_changed ||
  default_position?
end

assume_ is probably a confusing term, but we're using it in the form of 'put ourselves at', and in this case 'should we put ourselves at the default position'.

We do this if we're not already in the list (i.e. position is null), if we've changed scope into a new list but not changed the position, or if there's a database default for the position column and our position matches that (many people run without a database default for this column as it wasn't always supported).

If any of those are true then we need to set the position to the default, then push everyone else out of the way.

I suppose you could do a PR that adds a check to make sure the default position on the database matches the top_of_list, or perhaps override top_of_list to match the database default if it exists? Plus some documentation around this as it could be a breaking change for some.

I still think the branch is needed here :)

[eeeichan]

Sorry for the late reply.

Thank you for explaining the details of the process. I now understand the need for this process.

I suppose you could do a PR that adds a check to make sure the default position on the database matches the top_of_list

This was my original plan as another possibility. If I were to modify it, it would look something like this.

if assume_default_position? && (default_position == acts_as_list_top)

I will try to create a PR as soon as it is ready. Thank you for your continued support. We will close this PR as it is no longer needed as a result of the above exchange.

[brendon]

Thanks @eeeichan, I was more thinking of raising an error in this situation (so check on boot if the database is connected). I think there may already be a check like this for something else.

You could perhaps run a check in here: https://github.com/brendon/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/top_of_list_method_definer.rb and emit a warning in the Rails logger if you detect that a default position on the column and that value is higher than the top_of_list value:

acts_as_list_class.column_defaults[position_column.to_s] > top_of_list.to_i

Just be aware that sometimes the database isn't connected at this point so you have to be careful about that.