Add resource instance variables and audit log sample

[goosys]

I have made changes so that instance variables are stored in both the index and create actions, in order to capture controller operation logs and perform other post-processing tasks.
Please review.

[pablobm]

I'm ok with this, but I think it should be consistent and always be @resource (and @resources for the plural case).

Thoughts, @nickcharlton?

[nickcharlton]

Ah yeah, I see. Yeah, consistency is important here.

[goosys]

rebased

[nickcharlton]

Hah! It's so rare to see the use of a sprintf, fun!

[nickcharlton]

@pablobm, where you thinking that this should be @resource and not @new_resource? I'm not sure if we made any changes here after that comment

[pablobm]

That's right. Unless there's a reason for it, I think this should be @resource for consistency.

[pablobm]

Actually... now I'm torn. I forgot that we have @requested_resource, so having @resource might be confusing 🤔

[goosys]

https://github.com/goosys/administrate/blob/20cf3a7ab273e17312033ca1d3ed2e1c1503623f/app/controllers/concerns/administrate/resource_manager.rb#L24

I was reorganizing ApplicationController for a separate task, and I think @built_resource would be a good choice.

It clearly indicates that the resource is not yet saved, making it more explicit than simply using @resource.

[pablobm]

This makes me uneasy... 🤔 Increasing the number of instance variables increases complexity. In your audit_log for example, having to do if (resource = @requested_resource || @new_resource) is a smell. I would prefer to do if @resource. The action can be obtained from action if further filtering is required.

As mentioned, the presence of @requested_resource complicates things a bit... I'm looking at it now and I think that's the problem: a noun-method that should be a verb-method. I think instead we should do this in update, edit, destroy:

    def update
      @resource = find_resource(resource_params)
      if @resource
        redirect_to(
          after_resource_updated_path(@resource),
          notice: translate_with_resource("update.success"),
          status: :see_other
        )
      else
        render :edit, locals: {
          page: Administrate::Page::Form.new(dashboard, @resource)
        }, status: :unprocessable_entity
      end
    end

Which now relies on verb-method (also requires renaming the pre-existing find_resource):

    def find_resource
      find_resource_in_scope(params[:id]).tap do |resource|
        authorize_resource(resource)
      end
    end

    def find_resource_in_scope(param)
      scoped_resource.find(param)
    end

This is now consistent with index,new, and create, which retrieve the resource with a helper method and put it in an instance variable.

[goosys]

I liked the approach with @requested_resource, so I hadn't considered changing it. But now, as you mentioned, I think using @resource is better.

[pablobm]

And same thing here:

Suggested change

	@new_resource = resource = new_resource(resource_params)
	@resource = resource = new_resource(resource_params)

[goosys]

@nickcharlton @pablobm
If it's for the audit log, I feel like having @page would be nice too. Thoughts?

[pablobm]

Given how prevalent it is (it appears in all actions that render a page), I think it may make sense, yes 👍 Go for it.

[pablobm]

@goosys - Thoughts on this one? Particularly what I say at #2478 (comment)

[goosys]

@pablobm
I agree with the idea of standardizing everything to @resource!

[nickcharlton]

@goosys, I might be wrong, but it looks like we just need to settle on @resource here and then we can look at merging this. Could you take a look?

[goosys]

@pablobm @nickcharlton
I’ve unified the instance variables to @resource. How does this look?

The goal here is to make the resource available for post-processing in each action, so I think it’s reasonable to stop at this point for now.

We could consider renaming requested_resource separately in another step.

[nickcharlton]

Ah yeah, nice. I think that's it! Thanks!