Extract reports from controller #1250

isaacfreeman · 2016-06-15T05:18:11Z

When we have more than a handful of reports for a Solidus site, we typically end up with a long and messy decorator for the reports controller. This PR introduces a structure that allows us to extract the logic for each report into its own class, and declare which reports we want to be active in an initializer.

I appreciate this isn't ready to merge yet, but I'm opening the pull request for comment on whether this is a desirable approach. It's worked well for me with clients, but I don't know how universal my experience is. What do you think?

Notes:

Specs currently aren't passing
I've aimed to maintain backwards compatibility with existing decorators that use Spree::ReportsController.add_available_report!
There's some metaprogramming to generate an action in the controller for each generated report. This could perhaps be made less magical by simply using a show method that receives the desired report as a param, but this would break backwards compatibility.
I've added a second report showing variants that are out of stock. I'm not assuming that this specific report needs to also be merged into Solidus, and I'm open to removing it or moving it to a separate PR even if there's consensus on the general approach. I'll leave it in for now to illustrate how additional reports can be added.
The seed data currently doesn't have any out-of-stock variants, so you'll see no rows in the table until you make one. In the console for the sandbox app, run Spree::StockItem.first.set_count_on_hand(0).
The out-of-stock variants report uses a table_report view that's intended to be generally useful for any report that generates a table.

houndci-bot · 2016-06-15T05:18:18Z

backend/config/initializers/reports.rb

@@ -0,0 +1,3 @@
+  Rails.application.config.spree.add_class("reports")


Indentation of first line in file detected.

mtomov · 2016-06-21T21:11:26Z

I like the approach! My suggestions would be:

keep it to a single show action - it gets messy to have a controller action for every report. Metaprogramming helps indeed, but why not do it properly? I'm not sure that you will even be breaking compatibility, if you just add show, old reports with custom actions should theoretically work, no?
have a base Reporter class, which would define the common interface self.description, self.template, new, rows .., and can contain some helpful methods, such as def spree_routes; Spree::Core::Engine.routes.url_helpers; end;
rename def locals to something more revealing, like data, or something better
table report is great!

mtomov · 2016-06-21T21:12:31Z

backend/app/views/spree/admin/reports/table_report.html.erb

+  <table style="table-layout: fixed">
+    <colgroup>
+      <% @headers.each do |header| %>
+        <col style="width: <%= header.values.first.length * 0.8 %>em">


Wouldn't table-layout: fixed take care of the column widths?

isaacfreeman · 2016-06-21T21:44:12Z

@mtomov: Thanks! Agree on all your points. In particular, you're right about the show method – I was too quick to assume I couldn't do it for backward compatibility reasons, but of course you're right that it's not a problem.

How would you feel about simply using BaseReport as a parent class name rather than Reporter? It's closer to the domain object as understood by users, and the :base: prefix is used elsewhere in Solidus for similar purposes.

Also, would it be more convenient to continue with this PR, or close it while I follow up and open a new one later?

mtomov · 2016-06-21T21:49:48Z

Yes, either Spree::BaseReporter or maybe even Spree::Reports::Base. Maybe the singular is even better - Spree::Report::Base - your call.

By all means, do continue with this one. We can squash it all at the end.

houndci-bot · 2016-06-22T02:07:49Z

backend/app/models/spree/reports/sales_total.rb

+        Time.current.beginning_of_month
+      end
+
+      def parse_end_time(end_time)


Unused method argument - end_time. If it's necessary, use _ or _end_time as an argument name to indicate that it won't be used. You can also write as parse_end_time(*) if you want the method to accept any arguments but don't care about them.

- code specific to the Sales Total report mvoed to its own class - initializer to declare which reports should be available - reports controller uses define_method metaprogramming to set up an action for each report - GET and POST routes generated for each declared report

- optional title argument for Spree::Admin::ReportsController.add_available_report! - adds Spree::Reports::OutOfStockVariants - generic view file for table-based reports

- Removes redundant title code, in favour of just using the class name - Code improvements recommended by Hound

- plus some refactoring in SalesTotal

- uses a show method in ReportsController instead of generating an action for each report - extracts Spree::Reports::Base - fixes broken translation arguments in Spree::Reports::SalesTotal - removes unnecessary colgroup in table report view

- removes unused title key from ReportsController#add_available_report - adjusts translation namespace for ReportsController#add_available_report - adjusts ReportsControllerSpec for sales_total to use show action - code style improvements

…ther Solidus namespaces

mtomov · 2016-06-22T13:47:40Z

Btw, if we are to consider an external dependency, this gem Dossier was recommended to me.

isaacfreeman · 2016-06-28T23:56:53Z

@mtomov: Dossier looks good, but as you say it's an extra dependency, and I'd rather not add dependencies to the main Solidus distribution. However, if we go ahead with #1262 and move reports to their own spree_reports gem, I'd feel a lot more comfortable adding dependencies to that. We'd know then that people are opting in to using the reports, and we wouldn't be adding the dependency for people who'll never use it.

- moves SalesTotal report specs out of controller spec - controller specs for OutOfStockVariants report - handle case where report can't be found - adjustments to SalesTotal to better match existing specs

gmacdougall · 2016-07-08T13:20:07Z

backend/app/controllers/spree/admin/reports_controller.rb

-            report.content.each do |key, value|
-              instance_variable_set("@#{key}", value)
+        report_class = "Spree::Report::#{params[:id].camelize}".safe_constantize
+        if report_class.present?


This can just be if report_class

I've always found .present? a bit more readable, but I'm happy to match code style elsewhere in Solidus.

tvdeyen · 2017-10-04T12:01:08Z

We want to extract reports into it's own gem like stated in #1262

houndci-bot reviewed Jun 15, 2016
View reviewed changes

mtomov reviewed Jun 21, 2016
View reviewed changes

houndci-bot reviewed Jun 22, 2016
View reviewed changes

Isaac Freeman added 5 commits June 22, 2016 14:09

Adds second example report

68d5fc8

- optional title argument for Spree::Admin::ReportsController.add_available_report! - adds Spree::Reports::OutOfStockVariants - generic view file for table-based reports

Remove title

6bea7b5

- Removes redundant title code, in favour of just using the class name - Code improvements recommended by Hound

Renames #locals to #content

b6db41d

- plus some refactoring in SalesTotal

Improvements recommended by @mtomov

b8c7a57

- uses a show method in ReportsController instead of generating an action for each report - extracts Spree::Reports::Base - fixes broken translation arguments in Spree::Reports::SalesTotal - removes unnecessary colgroup in table report view

isaacfreeman mentioned this pull request Jun 22, 2016

Move reports to their own gem #1262

Closed

Isaac Freeman added 2 commits June 22, 2016 16:44

Updates to reports controller spec

931638d

- removes unused title key from ReportsController#add_available_report - adjusts translation namespace for ReportsController#add_available_report - adjusts ReportsControllerSpec for sales_total to use show action - code style improvements

Changes Spree::Reports module to Spree::Report for consistency with o…

fbc357f

…ther Solidus namespaces

isaacfreeman force-pushed the refactor/extract_reports_from_controller branch from c10905a to fbc357f Compare June 22, 2016 04:47

Spec updates

8ee1bd8

- moves SalesTotal report specs out of controller spec - controller specs for OutOfStockVariants report - handle case where report can't be found - adjustments to SalesTotal to better match existing specs

gmacdougall reviewed Jul 8, 2016
View reviewed changes

Adds spec for flash error when report isn't found

ac3fb2a

jhawthorn added this to the 2.2.0 milestone Dec 8, 2016

jhawthorn removed this from the 2.2.0 milestone Apr 13, 2017

tvdeyen closed this Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract reports from controller #1250

Extract reports from controller #1250

isaacfreeman commented Jun 15, 2016

houndci-bot Jun 15, 2016

mtomov commented Jun 21, 2016

mtomov Jun 21, 2016

isaacfreeman commented Jun 21, 2016

mtomov commented Jun 21, 2016

houndci-bot Jun 22, 2016

mtomov commented Jun 22, 2016

isaacfreeman commented Jun 28, 2016

gmacdougall Jul 8, 2016

isaacfreeman Jul 10, 2016

tvdeyen commented Oct 4, 2017

		@@ -0,0 +1,3 @@
		Rails.application.config.spree.add_class("reports")

Extract reports from controller #1250

Extract reports from controller #1250

Conversation

isaacfreeman commented Jun 15, 2016

houndci-bot Jun 15, 2016

Choose a reason for hiding this comment

mtomov commented Jun 21, 2016

mtomov Jun 21, 2016

Choose a reason for hiding this comment

isaacfreeman commented Jun 21, 2016

mtomov commented Jun 21, 2016

houndci-bot Jun 22, 2016

Choose a reason for hiding this comment

mtomov commented Jun 22, 2016

isaacfreeman commented Jun 28, 2016

gmacdougall Jul 8, 2016

Choose a reason for hiding this comment

isaacfreeman Jul 10, 2016

Choose a reason for hiding this comment

tvdeyen commented Oct 4, 2017