Add context to Page #2457

goosys · 2023-11-10T14:40:52Z

I have some proposal regarding Issue #2363.
I believe this PR can still be improved, so if anyone has any opinions, please review and let me know.

Administrate::Page#context

By keeping the context in the Page as follows,

page = Administrate::Page::Form.new(dashboard, requested_resource)
page.context = self # -> current controller

We can now define it as follows in the Dashboard.

ATTRIBUTE_TYPES = {
  catetory: Field::BelongsTo.with_options(
      scope: -> (field) { field.page.context.current_user.organization.categories }
  ),

With this, we can now differentiate the BelongsTo options based on the current user's permissions.

Also, by using context in form_attributes like this, we can now differentiate form elements based on the current user's permissions.

def form_attributes(action = nil, context = nil)
  if ["new", "create"].include?(action.to_s) && context.try(:pundit_user).try(:admin?)
    super
  else
    super - [:customer]
  end
end

Administrate::ApplicationController#contextualize_resource

I've prepared a hook point to add context to the resource.
This allows us to automatically set the current_user(pundit_user) to the customer form element that was omitted in the previous section.

def contextualize_resource(resource)
  if ["new", "create"].include?(action_name) && !pundit_user.admin?
    resource.customer = pundit_user
  end
end

Also, we can change the context here for models where the content of the validation changes under certain conditions.

class User < ApplicationRecord
  attr_accessor :inviting_by_admin
  validate :validate_invitation_code_matching, on: :create, unless: -> { inviting_by_admin.present? }
end

def contextualize_resource(resource)
  resource.inviting_by_admin = true if action_name == "create"
end

How do you think about?

goosys · 2023-11-10T14:46:22Z

Is it better to create Administrate::Page::Context because the contents of the context are too free and it's concerning?

pablobm

Apologies, it's been 4 months 😓 I'm starting to look into this now. I had a first quick look today, but couldn't quite get everything yet. I thought perhaps I can start asking questions and that way I'll force myself to be on top of this, if you are still available?

Thank you for the PR, by the way!

pablobm · 2024-04-12T14:10:10Z

app/controllers/administrate/application_controller.rb

@@ -62,8 +72,9 @@ def update
          notice: translate_with_resource("update.success"),
        )
      else
+        page = Administrate::Page::Form.new(dashboard, requested_resource)


Is this one missing the context assignment?

Suggested change

page = Administrate::Page::Form.new(dashboard, requested_resource)

page = Administrate::Page::Form.new(dashboard, requested_resource)

page.context = self

Ah, you're right. Sorry.

pablobm · 2024-04-12T14:15:09Z

app/controllers/administrate/application_controller.rb

+      authorize_scope(scoped_resource).find(param)
+    end
+
+    def authorize_scope(scope)


Perhaps this introduction of authorize_scope is off-topic in this PR? I can see the value of separating this concern, but it's different from the goal of adding context to the pages. What do you think?

Yes, you're right, this change is off-topic. I appreciate your accurate understanding of my intention.
However, I thought this change was also necessary for effective use of the contextualize_resource method, so I included it in the same PR. If necessary, I can split the PR.

pablobm · 2024-04-12T15:22:47Z

app/controllers/administrate/application_controller.rb

+    #
+    # @param resource [ActiveRecord::Base] A resource to be contextualized.
+    # @return nothing
+    def contextualize_resource(resource); end


I'm not sure that I understand this method. It's intended to make changes on the resource, but the context seems better suited to live in the Page object (which has access to the resource anyway). I'm not sure I would want to alter the model in this way.

Would you be able to explain this a bit more? 🙂

Thank you for your review!! I have been waiting for your response for a long time.

I would like to contextualize the Page object, but I also intend to contextualize the resource in the Controller. By contextualizing the resource, we can use it to branch validations or trigger callbacks.
I believe that contextualize_resource should be separate from the Page 's context, as it serves a similar function to scoped_resources and authorize_resource .

OK, I think I see now what is going on 👍

When contextualizing, we can hide fields and then put default values on them. In your example you do this to use the customer field from non-admins, then set its value to the current user on new/create.

I'm not convinced about the following:

Doing this as part of authorize_resource, which is a separate concern.

Doing this only when resource.is_a?(ActiveRecord::Base), which seems artificial. OK, it's not like we have currently support for anything other than ActiveRecord, but this explicit check seems strange to me.

Having this new API instead of the existing new_resource and requested_resource instead. It's two separate steps, but it uses existing, known APIs. I have tried locally and it appears to work (I'm having issues running specs so I can't be 100% sure).

Thank you for your review.

I have separated contextize_resource from authorize_resource. As a result, the resource no longer passes as a model class from the index, which also resolves the ActiveRecord::Base issue.

I believe the current form of requested_resource is ideal. What do you think? I would like to introduce the same form for new and create. Do you have any good suggestions on how to do this?

For example, what do you think about this form?

def built_resource(params: {}) @built_resource = new_resource(params).tap do |resource| authorize_resource(resource) contextualize_resource(resource) end end

I'm still not sure, since the same effect can be achieved by overriding new_resource and requested_resource as mentioned. I have created an experimental branch with your PR that shows an example of this: main...pablobm:use-cases-for-contextualize I'll experiment a bit to see what the possibilities are.

Shifting slightly to a different topic, I feel that the current action methods in the Administrate's ApplicationController are quite fat, which I believe limits the customizability for us, the application developers.
Therefore, it would be beneficial if these methods were broken down further, allowing us to override specific methods during application development.

I believe it would be ideal if we could proceed with method division and achieve a more unified form as shown below.
(It might be beneficial to also divide page = Administrate::Page::Form.new and the part where context is injected into Page into separate methods.)

def new page = Administrate::Page::Form.new(dashboard, built_resource) render ... def edit page = Administrate::Page::Form.new(dashboard, requested_resource) render ... def create if built_resource(resource_params).save ... def update if requested_resource.update(resource_params) ... def built_resource(params: {}) @built_resource ||= new_resource(params).tap do |resource| authorize_resource(resource) contextualize_resource(resource) end end def requested_resource @requested_resource ||= find_resource(params[:id]).tap do |resource| authorize_resource(resource) contextualize_resource(resource) end end

What do you think?

I think the other two commits are good ideas. Thank you.

The reason I prepared built_resource in addition to new_resource is because I thought it might affect the following process that is used as a helper_method from the view. Do you foresee any issues with this?
https://github.com/search?q=repo%3Athoughtbot%2Fadministrate+new_resource+language%3AHTML%2BERB&type=code&l=HTML%2BERB

pablobm · 2024-04-12T15:23:52Z

app/controllers/administrate/application_controller.rb

      resources = apply_collection_includes(resources)
      resources = order.apply(resources)
      resources = paginate_resources(resources)
      page = Administrate::Page::Collection.new(dashboard, order: order)
+      page.context = self


Following on my comment about contextualize_resource, I wonder if this is what it should be doing instead? Acting on the page instead of the resource here:

Suggested change

page.context = self

contextualize_page(page)

I think the Controller object is almost essential as a Context for the Page, so how about passing it to the initializer? Like the following:

page = Administrate::Page::Collection.new(dashboard, context: self, order: order) or page = Administrate::Page::Collection.new(dashboard, controller: self, order: order)

Initially that makes sense, but then the developers wouldn't be able to alter the context easily with a hook like contextualize_page. Having said that, perhaps there isn't a use case for that...? I have no idea yet. I'm experimenting now to see the possibilities of your PR.

pablobm

I had another look 🙂 While we are doing this, would you be able to rebase this in top of master, please? It will help me run the specs, as the way webdrivers are loaded has changed since the PR was created.

pablobm · 2024-04-26T15:10:37Z

spec/controllers/admin/orders_controller_spec.rb

+        post(
+          :create,
+          params: {
+            order: attributes_for(:order, customer: nil, customer_id: user.id),


Why not do this with the customer object? I feel it would be more natural. I had to read this a couple of times and check the factory to see what was going on:

Suggested change

order: attributes_for(:order, customer: nil, customer_id: user.id),

order: attributes_for(:order, customer: user),

Since this is a request parameter, I was unable to format it as customer: user.
However, this spec had many mistakes overall, so I have made corrections.

pablobm · 2024-04-26T15:11:02Z

spec/controllers/admin/orders_controller_spec.rb

+            order: attributes_for(:order, customer: nil, customer_id: user.id),
+          },
+        )
+      end


I would inline the method into the example, as it's only used once.

pablobm · 2024-04-26T15:55:36Z

app/controllers/administrate/application_controller.rb

+    #
+    # @param resource [ActiveRecord::Base] A resource to be contextualized.
+    # @return nothing
+    def contextualize_resource(resource); end


OK, I think I see now what is going on 👍

When contextualizing, we can hide fields and then put default values on them. In your example you do this to use the customer field from non-admins, then set its value to the current user on new/create.

I'm not convinced about the following:

Doing this as part of authorize_resource, which is a separate concern.

Doing this only when resource.is_a?(ActiveRecord::Base), which seems artificial. OK, it's not like we have currently support for anything other than ActiveRecord, but this explicit check seems strange to me.

Having this new API instead of the existing new_resource and requested_resource instead. It's two separate steps, but it uses existing, known APIs. I have tried locally and it appears to work (I'm having issues running specs so I can't be 100% sure).

pablobm · 2024-05-03T15:47:53Z

As mentioned in comments above, I have created a branch that uses your code and I'm experimenting with it. It's at main...pablobm:use-cases-for-contextualize. My goals are:

Work with your proposal so see how comfortable I am with the decisions.
See how it can address the issues listed at Provide more context to fields #2363, and what changes would be necessary in order to tackle all of them.

goosys force-pushed the feature/contextualize branch 5 times, most recently from cab44ec to 793187e Compare November 14, 2023 17:25

Holist mentioned this pull request Dec 19, 2023

Mise en place de pundit pour le super_admin et ajout du role support betagouv/rdv-service-public#3918

Merged

pablobm reviewed Apr 12, 2024

View reviewed changes

pablobm reviewed Apr 26, 2024

View reviewed changes

goosys force-pushed the feature/contextualize branch 3 times, most recently from 8d152bc to 4772467 Compare April 27, 2024 08:23

Add context to Page

83e593d

goosys force-pushed the feature/contextualize branch from 4772467 to 83e593d Compare April 27, 2024 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context to Page #2457

Add context to Page #2457

goosys commented Nov 10, 2023

goosys commented Nov 10, 2023

pablobm left a comment

pablobm Apr 12, 2024

goosys Apr 18, 2024

pablobm Apr 12, 2024

goosys Apr 18, 2024

pablobm Apr 12, 2024

goosys Apr 18, 2024

pablobm Apr 26, 2024

goosys Apr 27, 2024

pablobm May 3, 2024

goosys May 8, 2024

goosys May 8, 2024

goosys May 8, 2024 •

edited

pablobm Apr 12, 2024

goosys Apr 18, 2024

pablobm May 3, 2024

pablobm left a comment

pablobm Apr 26, 2024

goosys Apr 27, 2024

pablobm Apr 26, 2024

pablobm Apr 26, 2024

pablobm commented May 3, 2024

	page = Administrate::Page::Form.new(dashboard, requested_resource)
	page = Administrate::Page::Form.new(dashboard, requested_resource)
	page.context = self

	order: attributes_for(:order, customer: nil, customer_id: user.id),
	order: attributes_for(:order, customer: user),

Add context to Page #2457

Are you sure you want to change the base?

Add context to Page #2457

Conversation

goosys commented Nov 10, 2023

Administrate::Page#context

Administrate::ApplicationController#contextualize_resource

goosys commented Nov 10, 2023

pablobm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosys May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablobm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablobm commented May 3, 2024

goosys May 8, 2024 •

edited