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!

app/controllers/administrate/application_controller.rb

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.

Unresolving this as I think it's not settled. Currently I think this should be separated into a different PR. Perhaps in the context of your work on #2814

I’ve removed authorize_scope from this PR.
I’m planning to revisit it either in a separate PR or during a future refactor.

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 = 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.

spec/controllers/admin/orders_controller_spec.rb

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.

pablobm · 2024-06-07T09:35:50Z

I think this is the way to go, but there are some things still to polish. Let's see if I can make sense.

The main issue is #2363. This lists several other issues that require context. Therefore when we look for a solution we should find something that can tackle all the presented use cases, or at least enable a path towards tackling them in the future. Let's look at them one by one.

#1342 How to scope dropdown options to current_user in forms

I think this is addressed here, but the issue covers only part of the story. The proposed change tackles BelongsTo, but this should also extend to HasMany and HasOne. In fact, now I'm thinking it should extend to all field types, as I can envision similar issues with Select and possibly others.

#1586 Support for virtual fields (regression)

I'm not sure this one is actually related 😅 In any case, if all fields can get a context, then this one will benefit too.

#1862 Restricting specific fields visible in show/edit

There are several things here:

Show different attributes depending on context: this works for forms, but I think not for show and index. Let's add that too.
Ability to set certain fields depending on the context: I think this changes enables this behaviour by passing the context to permitted_attributes.
Same as 1, but for associations. I'm having a look and that might be tricky though. Not sure.

#1988 Filtering Field::HasMany

Already mentioned.

#2060 valid_actions? doesn't work for has_many relations

I think the context on permitted_attributes would help here too...?

Please let me know what you think!

goosys · 2024-06-08T19:51:52Z

#1342 How to scope dropdown options to current_user in forms
#1988 Filtering Field::HasMany

While it is possible to use the scope in Lambda format that administrate-field-scoped_belongs_to and administrate-field-scoped_has_many already introduce, it would be great to have the scope option available in Field::BelongsTo and Field::HasMany as well, considering the high demand for this feature.
Field::Select already has a collection option in Lambda format, so I believe it can still be used as-is even with the introduction of context.
Personally, I would be happy to see similar support for administrate-field-nested_has_many as well.

#1862 Restricting specific fields visible in show/edit

I have another suggestion regarding this.

In the edit screen, there are cases where users don't have permission to edit certain data but still need to see it.
For example, a BelongsTo field might be set by higher-level administrators and cannot be edited by regular administrators. However, not displaying this field in the edit screen would negatively affect usability. In the demo application, if we set the Customer field in the /orders/:id/edit screen to be hidden for regular administrators, and it disappears from the edit screen, users will not understand what they are trying to edit.

Do you have any ideas for mixing the _form partial and the _show partial in the edit screen? Of course, I would like to utilize context for this as well.

#2060 valid_actions? doesn't work for has_many relations

Isn't this the same issue as specific fields restrictions?
I think it would be sufficient to just add context to permitted_attributes, but it would be nice to have a more concise way to describe it.
Could Pundit's permitted_attributes_for_#{action} be a useful reference?
https://github.com/varvet/pundit/blob/main/lib/pundit/authorization.rb#L128

pablobm · 2024-06-18T04:30:27Z

I think we are in agreement here:

Yes to adding scope to all associative fields, as well as context to collection in Select.
Very good point about showing read-only fields in forms.
- I wonder if this could be inferred from the permit_attributes_for_#{action} that you propose.
- So for example: Order#customer is listed in FORM_ATTRIBUTES, but not allowed in permit_attributes_for_edit, so Administrate knows to show it read-only. Would that work? Would that be too complex or clever? I'll let you explore :-)
And indeed, permit_attributes_for_#{action} might be good for us here. I'll let you explore this one too!

goosys · 2024-07-30T13:52:47Z

@pablobm Is there anything else I can help with moving forward? If the scope of this PR is decided, I'm ready to commit to it until the end.

pablobm · 2024-08-02T09:14:52Z

@goosys - I think you are good to go, thank you! 👍 If it helps, you can let me know as you go and I can keep try keep an eye on your pushes to see how you are doing.

goosys · 2024-09-10T19:30:39Z

@pablobm I've created a PR for virtual fields. Please review it when you have time.

Support for virtual fields #2658

goosys · 2024-09-15T08:43:51Z

@pablobm
Apologies, I misunderstood in my previous comment. I noticed that HasMany also didn't have a callable scope option, so I added it.
For now, I copied the same implementation from other associative fields, but I feel that this is insufficient.

Since field is passed to the scope, the amount of code required to reach the context-aware resource is quite large.
For example, like this:

scope: -> (field) { field.page.resource.published_entries }

I would like to simplify it to something like the following:

scope: :published_entries

What do you think?

goosys · 2024-09-18T14:58:41Z

I’ve been testing this branch in my project, and I’m starting to feel that using only the Page context isn’t enough.
There were many cases where I wanted to adjust behavior based on the context in both the Dashboard and the Field. So, I’ve made an additional commit as an experiment.
753e224

Alternatively, I think this approach works fine for the Page.
4876803

What do you think?

goosys · 2024-10-30T06:34:12Z

I rebased after #2658 was merged.

goosys · 2025-02-22T08:41:38Z

I have rebased again.

@pablobm
Since some time has passed, I'd like to take a moment to review and organize this PR.
First, here’s what this PR accomplishes:

Page, Dashboard, and Field contextualization
- Page
  - Allows for conditional validation and similar logic
- Dashboard
  - Enables dynamic FORM_ATTRIBUTES based on the user
- Field
  - Allows dropdowns and other elements to be dynamic
Customizable validation contexts to ApplicationController
- Example: resource.save(context: validation_contexts_on_create)

As a result, I believe the status of the issues in #2363 is as follows.
Does this align with your understanding?

How to scope dropdown options to current_user in forms #1342
- ◎ Resolved
Support for virtual fields (regression) #1586
- ◎ Resolved in a separate PR Support for virtual fields #2658
Restricting specific fields visible in show/edit #1862
- △ Partially resolved
Filtering Field::HasMany #1988
- ◎ Resolved
valid_actions? doesn't work for has_many relations #2060
- △ Partially resolved
  - We haven't implemented any special features, but by introducing context to the Dashboard, we can now remove attributes that should not be updated based on the context from permitted_attributes. As a result, we were able to address at least the security concerns first.

For #1862 and #2060, we came up with new ideas during discussions.
Would it make sense to address them in the next PR instead?
Can we consider this PR complete once the contextualization is fully in place?

goosys · 2025-02-22T08:42:47Z

After this, I still have a few tasks I’d like to work on:

ApplicationController refactoring
- Standardizing slight variations in formatting across different actions
  - Add context to Page #2457 (comment)
Supporting non-HTML responses in ApplicationController
- Adjusting actions to work seamlessly for JSON API responses and CSV downloads
- ApplicationController refactoring #2814
Introducing callbacks at appropriate points in ApplicationController
- Previously, a hook was added only for successful create actions, but I found that approach inconsistent.
- I’d like to implement a unified callback system across all actions.

goosys · 2025-03-20T13:27:32Z

@nickcharlton
Could you also share the milestone for this? It's very important for my application as well. If there's anything else needed, I'm happy to help.

nickcharlton · 2025-03-20T16:39:33Z

I haven't been following this one because @pablobm has, but I just gave it a bit of a read to understand where we are and what we're trying to do and I think this is overall good. I'm also keen on your further ideas as well.

I'm really trying to keep the remaining bits of v1 focused on final testing before we cut the release, so I'm going to tag it post-v1. As for keeping your branches up to date (as you've been rebasing them a lot and I know how much of a pain it is!) I wouldn't expect much more to go in.

…pdate

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 4 times, most recently from 4772467 to 83e593d Compare April 27, 2024 10:50

goosys force-pushed the feature/contextualize branch from 83e593d to 8ef9389 Compare September 8, 2024 12:04

goosys force-pushed the feature/contextualize branch from 753e224 to 1d64c08 Compare October 30, 2024 06:31

goosys force-pushed the feature/contextualize branch 3 times, most recently from 781d936 to 7afd445 Compare November 5, 2024 07:50

goosys force-pushed the feature/contextualize branch from 7afd445 to b449732 Compare February 22, 2025 07:53

goosys force-pushed the feature/contextualize branch from b449732 to 0c5e538 Compare February 28, 2025 09:58

goosys requested a review from pablobm February 28, 2025 10:07

goosys force-pushed the feature/contextualize branch 3 times, most recently from 88e5ecb to a3ecb08 Compare March 4, 2025 16:05

goosys mentioned this pull request Mar 5, 2025

Display Non-Editable Fields on the Form for Better Context #2793

Open

goosys force-pushed the feature/contextualize branch from a3ecb08 to 011f06c Compare March 18, 2025 09:36

nickcharlton added feature new functionality that’s not yet implemented dashboards how administrate presents fields and displays data fields new fields, displaying and editing data labels Mar 20, 2025

nickcharlton added this to the Post v1 milestone Mar 20, 2025

goosys force-pushed the feature/contextualize branch 4 times, most recently from afbd64b to 1be075d Compare March 27, 2025 10:11

goosys mentioned this pull request Mar 27, 2025

Contextualize goosys/misty-bird-7370#54

Open

4 tasks

goosys added 5 commits April 11, 2025 06:01

Add context to Page

e2424bd

Add call-able :scope option to Field::HasMany

f465f16

Add context to Dashboard and Field

b169b3d

Add support for custom validation contexts in resource creation and u…

fef9abd

…pdate

Remove authorize_scope temporarily

b559109

goosys force-pushed the feature/contextualize branch from 66be5f3 to b559109 Compare April 11, 2025 06:07

Uh oh!

Add context to Page #2457

Are you sure you want to change the base?

Add context to Page #2457

Uh oh!

Conversation

goosys commented Nov 10, 2023

Administrate::Page#context

Administrate::ApplicationController#contextualize_resource

Uh oh!

goosys commented Nov 10, 2023

Uh oh!

pablobm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goosys May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablobm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablobm commented May 3, 2024

Uh oh!

pablobm commented Jun 7, 2024

#1342 How to scope dropdown options to current_user in forms

#1586 Support for virtual fields (regression)

#1862 Restricting specific fields visible in show/edit

#1988 Filtering Field::HasMany

#2060 valid_actions? doesn't work for has_many relations

Uh oh!

goosys commented Jun 8, 2024

Uh oh!

pablobm commented Jun 18, 2024

Uh oh!

goosys commented Jul 30, 2024

Uh oh!

pablobm commented Aug 2, 2024

Uh oh!

goosys commented Sep 10, 2024

Uh oh!

goosys commented Sep 15, 2024

Uh oh!

goosys commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goosys commented Oct 30, 2024

goosys May 8, 2024 •

edited

Loading

goosys commented Sep 18, 2024 •

edited

Loading

goosys commented Feb 22, 2025 •

edited

Loading

goosys commented Feb 22, 2025 •

edited

Loading