10000 Fix CSV responses when index overriding with the call alias method by megos · Pull Request #8031 · activeadmin/activeadmin · GitHub
[go: up one dir, main page]
Skip to content

Fix CSV responses when index overriding with the call alias method #8031

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix CSV responses when index overriding with the call alias method #8031

wants to merge 2 commits into from

Conversation

megos
Copy link
@megos megos commented Jul 26, 2023

CSV responses are implemented by overriding the index as below.

activeadmin/lib/active_admin/resource_controller/streaming.rb

Lines 12 to 17 in 2692838

def index
super do |format|
format.csv { stream_csv }
yield(format) if block_given?
end
end

If a user overrides the index using the alias method instead of the super, this implementation is ignored and CSV responses are not working.

controller do
  def index
    index! do |format| # Cause "No renderer defined for format: csv"
      ...
    end
  end
end

I add an alias method to make this implementation work anytime.

@megos megos force-pushed the fix-csv-responses-when-overriding-index branch from 0e3da46 to 23be363 Compare July 27, 2023 00:07
@javierjulio javierjulio force-pushed the fix-csv-responses-when-overriding-index branch from 23be363 to 0da137b Compare August 12, 2023 17:10
@mgrunberg mgrunberg force-pushed the fix-csv-responses-when-overriding-index branch from 2b5cee9 to 6775587 Compare September 26, 2024 21:42
@codecov Codecov
Copy link
codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (3b54bde) to head (6775587).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #8031   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4069     4070    +1     
=======================================
+ Hits         4033     4034    +1     
  Misses         36       36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgrunberg
Copy link
Contributor

@javierjulio this LGTM. What do you think?

@javierjulio
Copy link
Member

@mgrunberg not quite. It's why I held off but left this here as I'd like to see it resolved. I'm just not sure what the solution should be. I encountered this too the year before and resolved by calling super instead of index! inside the index action I had overridden. The following is the case I ran into:

controller do
  def index
    params[:some_flag] = "blah"
-   index!
+   super
  end
end

This is puzzling because I'm not sure why we are getting a new action method if InheritedResources would be defining this alias anyway since the Base applies the Actions module. Can you check if the controller has protected methods for show!, create!, etc.? I realized while responding, that this PR is setting a public alias but InheritedResources declares them as protected. Perhaps that would suffice here as I do recall one concern I had was the unexpected assertion change in the resource_controller_spec.rb file.

I'd like to see tests that have a custom index action that does something, e.g. set a param, before calling index! where a request is made for both html and csv to confirm it works as expected.

@mgrunberg
Copy link
Contributor

@javierjulio thanks for the feedback. I need some time to think about your comment. I'll come back to this in a few days. I don't discard even copy this PR as an "in repo".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jcpayvoice23 Jcpayvoice23 Jcpayvoice23 approved these changes

@javierjulio javierjulio Awaiting requested review from javierjulio

Assignees
No one assigned
Labels
type bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@megos @mgrunberg @javierjulio @Jcpayvoice23
0