Backend: more robust update_positions for resource controller

[hefan]

The persisted product_properties should be sortable per product. this did not work.

Reason:
It did not work because the non persistent dom_id's from the product property table pollutes the param string in the following way:

Parameters: {"positions"=>{"89"=>"1", "90"=>"2", "property"=>"3"}, "product_id"=>"ruby-hoodie"}

The dom_id helper used for sorting creates "spree_new_product_property" for the non persistent items table row. this is converted to the param "property"=>"3" above when positions are updated.
The default update_positions method from resources_controller rolls back the transaction so the position update does not work.

Possible Solutions where:

1. remove the creation of an id at all in the view. But i thought this wasn't a good option.

2. Override the update_positions method of the ProductPropertiesController (like its already done in the OptionTypesController).

i did the latter.

[kennyadsl]

Thanks, I left a question but other than that, I'm ok with this!

[kennyadsl]

Isn't this raising an exception calling set_list_position on nil if the find(id) can't find the record?

[hefan]

You are right, this record is not found.
But the resource controller catches the exception:
rescue_from ActiveRecord::RecordNotFound, with: :resource_not_found
But this doesn't hurt because the redirect of the json request to resource_not_found does nothing.

This is also why the update_position method in the resource controller didn't work and needed to be overriden. The difference is that the resource controller does the position updates in a Transaction and then does a rollback -> so no sorting happens!

i would say this is not the best solution to override the action.

so i did update this PR to do a cleanup of the update positions as a before_action.
Is that ok? Or is there a better solution?

[kennyadsl]

@hefan I'm taking a look here.

I can't find where we are already overriding the update_position method in Spree::Admin::OptionValuesController. Maybe you misread the update_values_positions name? That's done to allow ordering option values, but in the option types controller, see spree/spree#1707. The original method comes form Spree::Admin::ResourceController, and it's very similar to what you originally did.

I think we should find a generic solution that works for all the instances where we need to update the position of elements in the admin, probably, in Spree::Admin::ResourceController itself.

As first, thing, I'd add a spec that reproduces the issue, just to be sure it's reproducible and we don't reintroduce it later. We already have something here. I'm having positive results with the following code:

    context 'passing a not persisted item' do
      subject do
        post :update_positions, params: { id: widget_1.to_param,
          positions: { widget_1.id => '2', widget_2.id => '1', 'widget' => '3' }, format: 'js' }
       end

       it 'only updates the position of persisted attributes' do
         subject
         expect(Widget.all.order('position')).to eq [widget_2, widget_1]
       end
    end

supported by this code, that updates the update_position method in Spree::Admin::ResourceController:

  def update_positions
    ActiveRecord::Base.transaction do
      params[:positions].each do |id, index|
        model_class.find_by(id: id)&.set_list_position(index)
      end
    end

    respond_to do |format|
      format.js { head :no_content }
    end
  end

Note that now, it used find_by(id: id) that does not raise an exception (as find(id)) when the record is not found.

P.S.

Once we've done this, I'd probably also address not passing the params for non-persisted records in the JS code that make the ajax request by being sure we are passing a number

As a further improvement, we could also disable sorting for non-persisted rows, or remove the sorting handle for all the elements in the table to help users understand that until they save, sorting will be useless or not working as expected.

Of course, we can/should do this in other PRs.

Let me know if this makes sense for you as well, and thanks again. 🙏 🙏

[hefan]

Thank You. Yes, this sounds fine. i didn't want to touch the resource controller :).
This is done now in this PR.

About the OptionTypesControllers update_values_positions method: it is no override, but the same mechanism. It only works for now because the non persistent option value records are added above the none persistent ones with a copied persistent id and get overriden then by the persistent ones when updating positions. This we should address as well in later PRs.

In further PRs i would

1. remove the nonpersistent record in the ajax call like you said

then as discussed in #3561 for both product properties and option values:

2. add a remove button for non persistent fields
3. Add a default empty entry at the end
4. "Add new" produces new items below all other items (and the first new one)
5. do not add a sort option before save

[kennyadsl]

Thanks @hefan

[aldesantis]

I'm not a big fan of try, because it will swallow any errors even if the receiver of the method is not nil but it doesn't respond to the method. In this case, it will fail silently if find_by somehow returns an object that doesn't respond to set_list_position (e.g. because we remove it by mistake, or because something goes wrong with model_class and it returns the wrong class).

The correct syntax here would be the safe navigation operator (&.), which fails silently if the receiver is nil but returns an error in all other scenarios.

[kennyadsl]

Good catch, @aldesantis. I agree!

[hefan]

ok. the pull request is updated and uses the &. operator.

i thought the more verbose option would be nicer ;). Was not aware of the additional implications of .try vs &.