-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Action-specific models #1506
base: main
Are you sure you want to change the base?
Conversation
@nickcharlton, would you be able to have a look at this and see if it's worthwhile, before I spend more time on it? I want to make sure I'm not deviating from Administrate's spirit, or some similarly fundamental problem. |
@pablobm, I think this sounds like a good approach. Do you think you'd be able to add some draft documentation next so we can see how you might use this? (just a comment is fine, I think…) |
@nickcharlton I can help out with this, where do you suggest we put docs for this feature? Was looking at Edit: maybe another option would be putting a section in |
Thank you for your interest @cabe56. I had parked this PR while I worked on something else. I had envisioned two ways of documenting this feature:
Now, the problem is that Administrate doesn't yet have either! We haven't started documenting methods, and we don't have a "how to" section in the docs. I was thinking of doing that first so that this PR can get a place where to include its documentation. From your comment, I understand that you have an interest in this feature? If you could provide help, like a review, testing in your own projects, or a how-to document, that would be great. |
47b66f1
to
cb7cae5
Compare
@pablobm great ideas, I think that would help making Administrate more accessible. As you mention, these are not used in the project and I think incorporating them could be a barrier to getting this awesome feature shipped. Focused (short-term) on how to get your change through this review, I re-read @nickcharlton's suggestion and I think he meant just adding a quick snippet/comment/tl;dr on how to use this feature. I'll post back here once I try this PR out on my project. Thanks for taking the time to build this! |
Customer::Index | ||
else | ||
super | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a-ha. Here is the missing piece. Looks good.
A second approach could be placing something in CustomersDashboard
, maybe like:
action_classes = {
index: Customer::Index
}
...which would leave less for the developer to worry about, because all of the logic is in the dashboard class.
Maybe; I've been gone a while so I'm unsure if the second approach is sound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'm still unsure though, of where Administrate should be adding dashboard options and where it shouldn't. In general, the feature introduced in this PR feels to me like something that should be supported, but that in reality won't be used that often. For this reason, it doesn't necessarily deserve more explicit configuration options, as these become a maintenance burden later for little benefit.
It may still be a good idea, but the benefit of starting the feature at a controller level is that we can delay adding new dashboard-level options later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. Adding action-specific models seems like a niche usage, so overriding the controller is a good approach.
@original_product_policy = Product.policy_class | ||
Product.policy_class = TestProductPolicy | ||
@original_log_entry_policy = LogEntry.policy_class | ||
LogEntry.policy_class = TestLogEntryPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, glad you can re-use some code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your reviews @c4lliope. I left it as a draft while it got some initial reviews and I worked on other things. Currently I'm making an effort to pick up the drafts I've left behind and actually finalising them. At the moment I'm focused on #1941, and I plan to move on to this one when done.
Customer::Index | ||
else | ||
super | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'm still unsure though, of where Administrate should be adding dashboard options and where it shouldn't. In general, the feature introduced in this PR feels to me like something that should be supported, but that in reality won't be used that often. For this reason, it doesn't necessarily deserve more explicit configuration options, as these become a maintenance burden later for little benefit.
It may still be a good idea, but the benefit of starting the feature at a controller level is that we can delay adding new dashboard-level options later.
I don't remember why I closed this...? Re-opening for now, although I'm still a bit far from being able to work on it. |
This is an experiment to see what would be the simplest way to implement #278 The idea here is to use database views to render index pages, allowing us to display and sort records in any way we please.
For the moment, I managed to make it work while only having to change Administrate's
ApplicationController
. In this change, new hooks are introduced to allow controller actions to use specific resource classes. Therefore theindex
action can use a model based off a view, while other actions keep using the default model based off a table.As a result, this code can do three things:
lifetime_value
.This is not quite there yet. For one, I managed to make
index
actions to work this way, but notshow
actions. There are new things that break and I need to look into.Making the
show
action work would allow us to remove thelifetime_value
methods fromCustomer
andCustomer::Index
. The first is redundant with the view, and the second is only there to work around the first and avoid N+1 queries.I should also add some tests specific to the features listed above.
To do: