fix: actions and field hydration #1992

Paul-Bob · 2023-10-25T07:14:32Z

Description

Fixes #1946

This pull request addresses primarily two key issues: the class attributes for actions and the field hydration method.

Action class attributes

Relying on class attributes for actions is not convenient since undesired old values can remain populated on a new instance.

Field Hydration Method

The existing code made it impossible to hydrate the field with nil values, resulting in undesired behavior. For instance, in cases where a field within an action relied on the @record via a default Proc, it would correctly receive the default value when executed in a show view. However, when executed in an index view, the field attempted to hydrate with model: nil, yet the prior model would remain hydrated, causing the value to be fetched incorrectly.

`ActionModel` removed

ActionModel was not being used only to pass it as model on form_with helper, and I think that was unecessary.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
- actions warning docs.avohq.io#127
I have added tests that prove my fix is effective or that my feature works

Manual review steps

Create an action with several fields that have the default value as Proc reling on the @record
Example:

class ReleaseFish < Avo::BaseAction
  field :option_a, as: :boolean, default: -> { @record&.foo? }
  field :option_b, as: :boolean, default: -> { @record&.foo? }
  field :option_c, as: :boolean, default: -> { @record&.foo? }

  def handle(...)
    succeed ":)"
  end
end

Define the method on the model
Example:

  def foo?
    true
  end

Register the action on a resource
Go to that resource index page
Select 2 records
Run the action
Notice that all fields are false since @record is blank
Go to one record show view
Run the action
Notice that all fields are true since @record is setted ( and the default proc is running with @record properly hydrated on all executions )
Return to index and run action after selecting 2 records
Notice the same as step 7 (Prior this commit the fields would still hydrated with the @record from the show view since was impossible to hydrate with nil values)

Manual reviewer: please leave a comment with output from the test if that's the case.

codeclimate · 2023-10-25T07:14:58Z

Code Climate has analyzed commit e9ec6f3 and detected 0 issues on this pull request.

View more on Code Climate.

adrianthedev · 2023-10-29T21:24:37Z

lib/avo/fields/base_field.rb

-        @resource = resource if resource.present?
-        @action = action if action.present?
-        @user = user if user.present?
-        @panel_name = panel_name if panel_name.present?
+      def hydrate(**kwargs)
+        # List of permitted keyword argument keys as symbols
+        permited_kwargs_keys = %i[model resource action view panel_name user]

+        # Check for unrecognized keys
+        unrecognized_keys = kwargs.keys - permited_kwargs_keys
+        raise ArgumentError, "Unrecognized argument(s): #{unrecognized_keys.join(', ')}" if unrecognized_keys.any?
+
+        # Set instance variables with provided values
+        kwargs.each do |key, value|
+          instance_variable_set("@#{key}", value)
+        end


why the refactor here? it feels difficult to understand at a glance

From PR description:

The existing code made it impossible to hydrate the field with nil values, resulting in undesired behavior. For instance, in cases where a field within an action relied on the @record via a default Proc, it would correctly receive the default value when executed in a show view. However, when executed in an index view, the field attempted to hydrate with model: nil, yet the prior model would remain hydrated, causing the value to be fetched incorrectly.

What this does is if we use hydrate(model: nil) will assign @model = nil without touching the other variables... With the old code @model = model if model.present? since @model.present? == false the @model will keep the last assigned model even if we want to force hydrate it to nil...

adrianthedev · 2023-10-29T21:50:19Z

lib/avo/base_action.rb

+      @model = model
+      @resource = resource
+      @user = user
+      @view = view


Pfff. what a good fix!
THanks!

adrianthedev · 2023-10-29T21:50:43Z

app/controllers/avo/actions_controller.rb

@@ -16,7 +16,6 @@ def show
      @view = :new

      @resource.hydrate(model: @model, view: @view, user: _current_user, params: params)
-      @model = ActionModel.new @action.get_attributes_for_action


Not sure that this is the right decision.
I can't remember why, but at some point I know for a fact that it wasn't the right decision to use the @model.
Let's talk about it.

Tests are not failing, also tested it intensively manually and everything works as expected

adrianthedev

Ok, let's merge it but let's keep an eye on it and mention something in the upgrade guide that we changed something internal and if they see anything off, they should reach out to us.

Paul-Bob · 2023-10-31T13:53:03Z

Ok, let's merge it but let's keep an eye on it and mention something in the upgrade guide that we changed something internal and if they see anything off, they should reach out to us.

avo-hq/docs.avohq.io#127

github-actions · 2023-10-31T13:53:56Z

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

fix: actions and field hydration

2561582

Paul-Bob added the Fix label Oct 25, 2023

Paul-Bob requested a review from adrianthedev October 25, 2023 07:14

Paul-Bob self-assigned this Oct 25, 2023

ActionModel for actions show

2150ed2

Paul-Bob marked this pull request as draft October 25, 2023 07:59

wip

e9ec6f3

Paul-Bob marked this pull request as ready for review October 25, 2023 16:59

adrianthedev reviewed Oct 29, 2023

View reviewed changes

adrianthedev approved these changes Oct 31, 2023

View reviewed changes

Paul-Bob merged commit f7acbe4 into main Oct 31, 2023
12 of 13 checks passed

Paul-Bob deleted the fix/actions_and_field_hydration branch October 31, 2023 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: actions and field hydration #1992

fix: actions and field hydration #1992

Paul-Bob commented Oct 25, 2023 •

edited

Loading

codeclimate bot commented Oct 25, 2023 •

edited

Loading

adrianthedev Oct 29, 2023

Paul-Bob Oct 30, 2023 •

edited

Loading

Paul-Bob Oct 30, 2023

adrianthedev Oct 29, 2023

adrianthedev Oct 29, 2023

Paul-Bob Oct 31, 2023

adrianthedev left a comment

Paul-Bob commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

fix: actions and field hydration #1992

fix: actions and field hydration #1992

Conversation

Paul-Bob commented Oct 25, 2023 • edited Loading

Description

Action class attributes

Field Hydration Method

ActionModel removed

Checklist:

Manual review steps

codeclimate bot commented Oct 25, 2023 • edited Loading

adrianthedev Oct 29, 2023

Choose a reason for hiding this comment

Paul-Bob Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Paul-Bob Oct 30, 2023

Choose a reason for hiding this comment

adrianthedev Oct 29, 2023

Choose a reason for hiding this comment

adrianthedev Oct 29, 2023

Choose a reason for hiding this comment

Paul-Bob Oct 31, 2023

Choose a reason for hiding this comment

adrianthedev left a comment

Choose a reason for hiding this comment

Paul-Bob commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

Paul-Bob commented Oct 25, 2023 •

edited

Loading

`ActionModel` removed

codeclimate bot commented Oct 25, 2023 •

edited

Loading

Paul-Bob Oct 30, 2023 •

edited

Loading