Uk/1316 ui grid reports #1547

jeronimo · 2017-04-25T14:46:16Z

Order and Distributors, Bulk Coop, Orders and Fulfillments have UI grid design reports.
It is possible to switch to old version as well.

TODO:

UX and people using reports need to decide if this is appropriate approach, which columns should be visible or not needed. And if any additional functionality is needed.
Replace other reports with the same styling.
To get rid off old versions of reports and refactor the code.

…vices

…bject and 'all' array

… services

…of data into byID and pristineByID

…levant info in group headers

…tifications

…ranslations

lin-d-hop · 2017-05-07T18:00:54Z

Looks like some merge conflicts.... Could you take a look please @jeronimo?

jeronimo · 2017-05-08T08:01:40Z

Fixed!

daniellemoorhead · 2017-05-12T01:20:41Z

Still showing up with conflicts :(

jeronimo · 2017-05-12T06:44:54Z

Ok, fixed conflicts again. But there is another test failing in Travis which is not related with the code and it fails only in Travis :(

jeronimo · 2017-05-12T07:58:14Z

Ok, it passed!

daniellemoorhead · 2017-05-19T01:33:08Z

Awesome! Over to you @oeoeaio

daniellemoorhead · 2017-06-02T01:24:44Z

Hey @jeronimo, very big apologies that we haven't looked at this yet. @oeoeaio is working on your smaller PRs this week, and will dedicate a day next week to looking at this.

oeoeaio · 2017-06-16T01:54:50Z

Hey @jeronimo, just a general comment: I would really like to avoid having merge commits clogging up the commit history (eg. fdb1c59, 954530b, c60182c, 097636e and a89a08f) especially when they are just being used keep the branch up to date. With feature branches like this, rebasing against the current version of master is a much better approach. Could you please make sure that you are rebasing rather than merging in future?

oeoeaio

Hey @jeronimo, thanks so much for all of your work on this! It's looking great! Ping me if you want anything clarified. :)

oeoeaio · 2017-06-16T03:53:21Z

app/controllers/spree/admin/reports_controller_decorator.rb

@@ -181,20 +189,37 @@ def sales_tax
  end

  def bulk_coop
-    # -- Prepare date parameters
-    prepare_date_params params
+    if request.format.json?


I would anticipate that the structure of this controller would ultimately change completely, so that all html requests would be caught in a universal before_filter that simply rendered the relevant html UI. Then, all of the main controller logic would be written to cater for JSON API requests made from html UI. Does that sound about right @jeronimo? Would that be acceptable @enricostano, or are you concerned about having a single controller handling both standard and API requests?

oeoeaio · 2017-06-16T03:54:59Z

app/controllers/spree/admin/reports_controller_decorator.rb

+        products: @report.products_serialized,
+        distributors: @report.distributors_serialized,
+        variants: @report.variants_serialized }
+      render json: report_data


This part looks good 👍

oeoeaio · 2017-06-16T04:01:17Z

app/controllers/spree/admin/reports_controller_decorator.rb

      end
-      send_data csv_string, :filename => "orders_and_distributors_#{timestamp}.csv"
    end


I am assuming that we will ultimately remove the html section right? We won't need the csv functionality and we don't need to load any data to render the form.

We'll need to load the date params in orders to render the form, but perhaps we can do that in a before_filter?

oeoeaio · 2017-06-16T04:01:34Z

app/controllers/spree/admin/reports_controller_decorator.rb

+        products: @report.products_serialized,
+        distributors: @report.distributors_serialized,
+        variants: @report.variants_serialized }
+      render json: report_data


oeoeaio · 2017-06-16T04:18:31Z

app/assets/javascripts/admin/index_utils/directives/datepicker.js.coffee

@@ -7,3 +7,5 @@ angular.module("admin.indexUtils").directive "datepicker", ->
        scope.$apply (scope) ->
          # Fires ngModel.$parsers
          ngModel.$setViewValue dateText
+    if attrs.value
+      ngModel.$setViewValue attrs.value


This isn't really what this directive is for. If you need to change the datepicker elements to rails form elements rather than pure angular elements, I would suggest removing the ng-model directive altogether (it is doing nothing) and using our custom watch-value-as directive instead.

That said, my preference (instead of c35a165) would be to inject the search data as a service, and have everything be pure angular from there on. I don't like hacking rails form elements to make them 'work' with angular. Too many pitfalls, and too hard to work out what is going wrong when you have multiple custom directives all trying to manipulate the value of the form element directly.

oeoeaio · 2017-06-16T07:14:58Z

spec/lib/open_food_network/order_and_distributor_report_spec.rb

      end

-      it "should denormalise order and distributor details for display as csv" do
-        subject = OrderAndDistributorReport.new [@order]
+      context "Old version" do


How to you feel about describing these contexts as html and json instead of old and new?

oeoeaio · 2017-06-16T07:16:26Z

spec/lib/open_food_network/reports/base_report_spec.rb

+            expect(report.distributors_serialized.options[:each_serializer]).to eq(Api::Admin::Reports::EnterpriseSerializer)
+          end
+        end
+


Remove this blank space, and the one below

oeoeaio · 2017-06-16T07:18:00Z

spec/lib/open_food_network/reports/base_report_spec.rb

+        before { order.line_items << line_item }
+
+        it '#table_items returns one line_item' do
+          expect(report.table_items).to eq([line_item])


I would normally do .to eq [line_item]. Do you have a preference for doing it with parentheses? Doesn't really matter, I'm more thinking about what we do as a convention.

oeoeaio · 2017-06-16T07:21:25Z

spec/serializers/admin/reports/line_item_serializer_spec.rb

+  #   result['tags'].each do |tag|
+  #     expect(tag['rules']).to be nil
+  #   end
+  # end


oeoeaio · 2017-06-16T07:21:35Z

spec/serializers/admin/reports/line_item_serializer_spec.rb

+    it '#paid?' do
+      expect(subject.paid?).to eq "No"
+    end
+


oeoeaio · 2017-06-18T02:28:20Z

app/controllers/spree/admin/reports_controller_decorator.rb

+        orders: @report.orders_serialized,
+        products: @report.products_serialized,
+        variants: @report.variants_serialized }
+      render json: report_data


It would be cool to use the OrdersAndFulfillmentSerializer here:

render json: @report, serializer: Api::Admin::Reports::OrdersAndFulfillmentSerializer

Then you could remove the line_items_serialized, orders_serialized methods from the report, and the report_data hash here. I think this is what I was intending to do in my spike, but I never got around to it.

sauloperez · 2017-07-10T08:57:23Z

app/serializers/api/admin/reports/order_serializer.rb

@@ -0,0 +1,63 @@
+class Api::Admin::Reports::OrderSerializer < ActiveModel::Serializer


Could you describe at a high level how you're designing this? I don't really understand why we are adding this new set of serializers or how the current reports in lib/open_food_network are related to this PR.

@sauloperez the basic idea is that the user loads a report as html, and then a separate request for report data is sent via AJAX to ReportsController which uses the lib/open_food_network classes to load data for displaying in different classes of report, this data is then serialised and rendered as JSON, which is consumed by the ui-grids library and rendered dynamically on the page.

Is that the way ui-grids works? Sounds a bit weird to me having to load the HTML and the data separately in two different requests.

Sorry, maybe I wasn't clear, I don't mean that ui-grids needs to receive JSON, I just mean that it takes data and renders it in the DOM. ui-grid isn't continuously pinging the server for data or anything like that.

lin-d-hop · 2017-12-28T01:42:03Z

Closing with reports upgrade on the back burner

RohanM and others added 30 commits October 27, 2016 11:57

Cherry-pick b2d82b6 - Using ofnSelect2 instead of plain select on BOM

9ca5341

Refactoring ofnSelect2, works with multiple, and with promised data

ad76528

Expunge all mentions of includeBlank and blankOption from angular ser…

009bf0b

…vices

Changing convention for angular resource services to generic 'byID' o…

8c44add

…bject and 'all' array

Creating new 'resources' module for holding interdependent ngResource…

d33df30

… services

Adding #load function to services for resources, for generic loading …

74f644b

…of data into byID and pristineByID

SPIKE: Initial cut at adding UI-Grid

2c287b8

SPIKE: Basic grid showing data

1f552d2

SPIKE: Sorting out ui-grid fonts

206715e

SPIKE: shifting uigrid inclusion in css manifest (tiny change)

c704925

SPIKE: Adding more columns for demonstration

cd5c4b7

SPIKE: Using treeView instead of grouping

9dbc135

SPIKE: Back to grouping with ui-grid, using aggregations to diplay re…

346a8f3

…levant info in group headers

SPIKE: Getting filters working on orders and fulfillments report page

fa911f3

SPIKE: Export as CSV using ui-grid

fd584bb

Merge oeoeaio/ui-grid-reports

d566086

Copy paste js files for orders and distributors

a92071c

Working Orders and Distributors report mockup

b1d69be

Orders and Distribution: Make SKU visible for items

444ee92

Merge branch 'master' into uk/1316-ui-grid-reports

a89a08f

Add PdfMake lib for ui-grid PDF export

8d12518

Orders and distributions: show all fields and group properly

1328431

Orders and distributions: enable grid menu and PDF download

da10c01

Orders and distributions: set MaxGridWidth for PDF export

457cbfd

Orders and distributions: Add "Loading..." and "No data available" no…

9247066

…tifications

Orders and distributions: Add default start date for filter

c35a165

Orders and distributions: Add default start date for filter && some t…

1e7406a

…ranslations

Add capybara-screenshot gem

689afd0

Orders and distributors: Cleanup code and fix tests

12e67db

Add pdfmake js map

cceba09

daniellemoorhead requested a review from oeoeaio May 3, 2017 02:59

daniellemoorhead modified the milestones: Next small release (1.8.11 or 1.9.1), v1.8.10 - Cock & Hen May 3, 2017

Merge branch 'master' into uk/1316-ui-grid-reports

c60182c

Merge branch 'master' into uk/1316-ui-grid-reports

954530b

daniellemoorhead added the aus review label May 12, 2017

Merge branch 'master' into uk/1316-ui-grid-reports

fdb1c59

Replace some rspec syntax to expect and cleanup gemfile.lock

9546123

daniellemoorhead modified the milestones: June dot point release, May dot point release May 24, 2017

oeoeaio suggested changes Jun 16, 2017

View reviewed changes

oeoeaio reviewed Jun 18, 2017

View reviewed changes

sigmundpetersen mentioned this pull request Jun 22, 2017

Report Filtering - minor feature broken #1473

Closed

daniellemoorhead modified the milestones: July dot point release, June dot point release Jun 23, 2017

daniellemoorhead removed the aus review label Jun 30, 2017

sauloperez reviewed Jul 10, 2017

View reviewed changes

daniellemoorhead removed this from the July dot point release milestone Jul 21, 2017

leandroalemao mentioned this pull request Sep 16, 2017

Uk/1316 ui grid reports rebased #1836

Closed

daniellemoorhead removed the reports label Nov 10, 2017

lin-d-hop closed this Dec 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uk/1316 ui grid reports #1547

Uk/1316 ui grid reports #1547

jeronimo commented Apr 25, 2017 •

edited

Loading

lin-d-hop commented May 7, 2017

jeronimo commented May 8, 2017

daniellemoorhead commented May 12, 2017

jeronimo commented May 12, 2017

jeronimo commented May 12, 2017

daniellemoorhead commented May 19, 2017

daniellemoorhead commented Jun 2, 2017

oeoeaio commented Jun 16, 2017

oeoeaio left a comment

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 16, 2017

oeoeaio Jun 18, 2017

sauloperez Jul 10, 2017

oeoeaio Jul 27, 2017

sauloperez Jul 27, 2017 •

edited

Loading

oeoeaio Jul 27, 2017 •

edited

Loading

lin-d-hop commented Dec 28, 2017

		@@ -0,0 +1,63 @@
		class Api::Admin::Reports::OrderSerializer < ActiveModel::Serializer

Uk/1316 ui grid reports #1547

Uk/1316 ui grid reports #1547

Conversation

jeronimo commented Apr 25, 2017 • edited Loading

lin-d-hop commented May 7, 2017

jeronimo commented May 8, 2017

daniellemoorhead commented May 12, 2017

jeronimo commented May 12, 2017

jeronimo commented May 12, 2017

daniellemoorhead commented May 19, 2017

daniellemoorhead commented Jun 2, 2017

oeoeaio commented Jun 16, 2017

oeoeaio 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

oeoeaio Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

lin-d-hop commented Dec 28, 2017

jeronimo commented Apr 25, 2017 •

edited

Loading

sauloperez Jul 27, 2017 •

edited

Loading

oeoeaio Jul 27, 2017 •

edited

Loading