read rendering options directly from the model ReportRendingOptions

Revert "Cache address JSON to avoid database queries"

.github/ISSUE_TEMPLATE/bug_report.md

@@ -2,7 +2,7 @@
 name: Bug report
 about: Create a report to help us improve
 title: ''
-labels: tech debt
+labels: ''
 assignees: ''
 
 ---

Gemfile.lock

@@ -159,7 +159,7 @@ GEM
       activerecord (>= 4.0.0, < 7.1)
     aws-eventstream (1.2.0)
     aws-partitions (1.651.0)
-    aws-sdk-core (3.165.1)
+    aws-sdk-core (3.166.0)
       aws-eventstream (~> 1, >= 1.0.2)
       aws-partitions (~> 1, >= 1.651.0)
       aws-sigv4 (~> 1.5)
@@ -167,15 +167,12 @@ GEM
     aws-sdk-kms (1.59.0)
       aws-sdk-core (~> 3, >= 3.165.0)
       aws-sigv4 (~> 1.1)
-    aws-sdk-s3 (1.117.0)
+    aws-sdk-s3 (1.117.1)
       aws-sdk-core (~> 3, >= 3.165.0)
       aws-sdk-kms (~> 1)
       aws-sigv4 (~> 1.4)
     aws-sigv4 (1.5.2)
       aws-eventstream (~> 1, >= 1.0.2)
-    axlsx_styler (1.1.0)
-      activesupport (>= 3.1)
-      caxlsx (>= 2.0.2)
     bcrypt (3.1.18)
     bigdecimal (3.0.2)
     bindata (2.4.12)
@@ -206,7 +203,7 @@ GEM
       rack-test (>= 0.6.3)
       regexp_parser (>= 1.5, < 3.0)
       xpath (~> 3.2)
-    caxlsx (3.1.1)
+    caxlsx (3.3.0)
       htmlentities (~> 4.3, >= 4.3.4)
       marcel (~> 1.0)
       nokogiri (~> 1.10, >= 1.10.4)
@@ -268,7 +265,6 @@ GEM
     dotenv-rails (2.8.1)
       dotenv (= 2.8.1)
       railties (>= 3.2)
-    dry-inflector (0.2.1)
     erubi (1.11.0)
     et-orbi (1.2.7)
       tzinfo
@@ -565,9 +561,8 @@ GEM
     roadie-rails (3.0.0)
       railties (>= 5.1, < 7.1)
       roadie (~> 5.0)
-    rodf (1.1.1)
+    rodf (1.2.0)
       builder (>= 3.0)
-      dry-inflector (~> 0.1)
       rubyzip (>= 1.0)
     roo (2.9.0)
       nokogiri (~> 1)
@@ -657,9 +652,8 @@ GEM
       simplecov_json_formatter (~> 0.1)
     simplecov-html (0.12.3)
     simplecov_json_formatter (0.1.3)
-    spreadsheet_architect (4.2.0)
-      axlsx_styler (>= 1.0.0, < 2)
-      caxlsx (>= 2.0.2, < 4)
+    spreadsheet_architect (5.0.0)
+      caxlsx (>= 3.3.0, < 4)
       rodf (>= 1.0.0, < 2)
     spring (4.1.0)
     spring-commands-rspec (1.0.4)

README.md

@@ -35,7 +35,13 @@ We also have a [Super Admin Guide][super-admin-guide] to help with configuration
 
 If you'd like to help out with testing, please introduce yourself on the #testing channel on [Slack][slack-invite] and download the [ZenHub browser extension][zenhub] to view the development pipeline. Also, do have a look in our [Welcome New QAs board](https://github.com/orgs/openfoodfoundation/projects/1) for some good first issues, both on manual and automated testing (RSpec/Capybara).
 
-We use [BrowserStack](https://www.browserstack.com/) as a manual testing tool. BrowserStack provides open source projects with unlimited and free of charge accounts. A big thanks to them! 
+We use [BrowserStack](https://www.browserstack.com/) as a manual testing tool. BrowserStack provides open source projects with unlimited and free of charge accounts. A big thanks to them!
+
+
+We use [KnapsackPro](https://knapsackpro.com/) for optimal parallelisation of our automated tests. KnapsackPro offers unlimited plans for non-commercial open source projects, like ours - a big thanks to them!
+
+![image](https://user-images.githubusercontent.com/49817236/201330047-e64147a7-d91c-4c10-bd4d-ca519d8fe945.png)
+
 
 ## Licence
 

app/controllers/admin/reports_controller.rb

@@ -48,15 +48,7 @@ module Admin
                       else
                         I18n.t(:name, scope: [:admin, :reports, @report_type])
                       end
-
-      # Initialize data
-      params[:display_summary_row] = true if request.get?
-      @params_fields_to_show = if request.get?
-                                 @report.columns.keys - @report.fields_to_hide
-                               else
-                                 params[:fields_to_show]
-                               end
-
+      @rendering_options = rendering_options
       @data = Reporting::FrontendData.new(spree_current_user)
     end
   end

app/controllers/concerns/reports_actions.rb

@@ -62,4 +62,27 @@ module ReportsActions
   def i18n_scope
     'admin.reports'
   end
+
+  def rendering_options
+    @rendering_options ||= ReportRenderingOptions.where(
+      user: spree_current_user,
+      report_type: report_type,
+      report_subtype: report_subtype
+    ).first_or_create do |new_instance|
+      new_instance.options[:fields_to_show] = if @report.present?
+                                                @report.columns.keys - @report.fields_to_hide
+                                              else
+                                                []
+                                              end
+      new_instance.options[:display_summary_row] = request.get? || params[:display_summary_row].present?
+      params[:display_header_row] = params[:display_header_row].present?
+    end
+    if params[:fields_to_show].present?
+      @rendering_options.options[:fields_to_show] = params[:fields_to_show]
+    end
+    @rendering_options.options[:display_summary_row] = params[:display_summary_row].present?
+    @rendering_options.options[:display_header_row] = params[:display_header_row].present?
+    @rendering_options.save
+    @rendering_options
+  end
 end

app/models/enterprise.rb

@@ -16,19 +16,6 @@ class Enterprise < ApplicationRecord
     large: { resize_to_fill: [1200, 260] },
   }.freeze
 
-  self.ignored_columns = %i(terms_and_conditions_file_name
-                            terms_and_conditions_content_type
-                            terms_and_conditions_file_size
-                            terms_and_conditions_updated_at
-                            logo_file_name
-                            logo_content_type
-                            logo_file_size
-                            logo_updated_at
-                            promo_image_file_name
-                            promo_image_content_type
-                            promo_image_file_size
-                            promo_image_updated_at)
-
   searchable_attributes :sells, :is_primary_producer
   searchable_associations :properties
   searchable_scopes :is_primary_producer, :is_distributor, :is_hub, :activated, :visible,

app/models/enterprise_group.rb

@@ -5,15 +5,6 @@ require 'open_food_network/locking'
 class EnterpriseGroup < ApplicationRecord
   include PermalinkGenerator
 
-  self.ignored_columns = %i(logo_file_name
-                            logo_content_type
-                            logo_file_size
-                            logo_updated_at
-                            promo_image_file_name
-                            promo_image_content_type
-                            promo_image_file_size
-                            promo_image_updated_at)
-
   acts_as_list
 
   has_and_belongs_to_many :enterprises, join_table: 'enterprise_groups_enterprises'

app/models/product_import/entry_validator.rb

@@ -299,7 +299,7 @@ module ProductImport
         entry.public_send("#{type}_category_id=", index[category])
       else
         mark_as_invalid(entry, attribute: "#{type}_category",
-                               error: I18n.t('admin.product_import.model.not_found'))
+                               error: I18n.t('admin.product_import.model.category_not_found'))
       end
     end
 

app/models/product_import/spreadsheet_entry.rb

@@ -68,7 +68,7 @@ module ProductImport
 
     def invalid_attributes
       invalid_attrs = {}
-      errors = @product_validations ? self.errors.messages.merge(@product_validations.messages) : self.errors.messages
+      errors = @product_validations ? @product_validations.messages.merge(self.errors.messages) : self.errors.messages
       errors.each do |attr, message|
         invalid_attrs[attr.to_s] = "#{attr.to_s.capitalize} #{message.first}"
       end

app/models/report_rendering_options.rb

@@ -0,0 +1,4 @@
+class ReportRenderingOptions < ApplicationRecord
+  belongs_to :user, class_name: "Spree::User"
+  serialize :options, Hash
+end

app/models/spree/image.rb

@@ -9,13 +9,6 @@ module Spree
       large: { resize_to_limit: [600, 600] },
     }.freeze
 
-    self.ignored_columns = %i(attachment_file_name
-                              attachment_content_type
-                              attachment_file_size
-                              attachment_updated_at
-                              attachment_width
-                              attachment_height)
-
     has_one_attached :attachment
 
     validates :attachment, attached: true, content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z}

app/models/terms_of_service_file.rb

@@ -5,11 +5,6 @@ class TermsOfServiceFile < ApplicationRecord
 
   validates :attachment, attached: true
 
-  self.ignored_columns = %i(attachment_file_name
-                            attachment_content_type
-                            attachment_file_size
-                            attachment_updated_at)
-
   # The most recently uploaded file is the current one.
   def self.current
     order(:id).last

app/views/admin/enterprises/_form.html.haml

@@ -5,12 +5,6 @@
       %legend= t(".#{ item[:name] }.legend")
       = render "admin/enterprises/form/#{ item[:form_name] || item[:name] }", f: f
 
-  - when 'address'
-    = f.fields_for :address do |af|
-      %fieldset.alpha.no-border-bottom{ id: "#{item[:name]}_panel", data: { "tabs-and-panels-target": "panel" }}
-        %legend= t(".#{ item[:name] }.legend")
-        = render 'admin/enterprises/form/address', af: af
-
   - when 'enterprise_permissions'
     %fieldset.alpha.no-border-bottom{ id: "#{item[:name]}_panel", data: { "tabs-and-panels-target": "panel" }}
       %legend= t(".#{ item[:name] }.legend")

app/views/admin/enterprises/form/_address.html.haml

@@ -1,54 +1,55 @@
 -# redo denoting required fields in the whole project
-.row
-  = t(:required_fields)
-  (
-  %span.required *
-  )
-.row
-  .three.columns.alpha
-    = af.label :address1, t(:address)
+= f.fields_for :address do |af|
+  .row
+    = t(:required_fields)
+    (
     %span.required *
-  .eight.columns.omega
-    = af.text_field :address1, { placeholder: t(:address_placeholder) }
-.row
-  .alpha.three.columns
-    = af.label :address2, t(:address2)
-  .eight.columns.omega
-    = af.text_field :address2
-.row
-  .three.columns.alpha
-    = af.label :city, t(:city)
-    \/
-    = af.label :zipcode, t(:postcode)
-    %span.required *
-  .four.columns
-    = af.text_field :city, { placeholder: t(:city_placeholder) }
-  .four.columns.omega
-    = af.text_field :zipcode, { placeholder: t(:postcode_placeholder) }
-.row{"data-controller": "dependent-select", "data-dependent-select-options-value": countries_with_states }
-  .three.columns.alpha
-    = af.label :state_id, t(:state)
-    \/
-    = af.label :country_id, t(:country)
-    %span.required *
-  .four.columns
-    = af.select :state_id, @enterprise.address.country.states.map { |s| [s.name, s.id] }, {}, { "data-controller": "tom-select", "data-dependent-select-target": "select", class: "primary" }
-  .four.columns.omega{ data: { controller: "primary-details" }}
-    = af.select :country_id, available_countries.map { |c| [c.name, c.id] }, {}, { "data-controller": "tom-select", "data-dependent-select-target": "source", "data-action": "dependent-select#handleSelectChange", class: "primary" }
-.row
-  .three.columns.alpha
-    = af.label :latitude, t(:latitude)
-    \/
-    = af.label :longitude, t(:longitude)
-    %span.required *
-    = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('latitude_longitude_tip')}
-  .four.columns
-    = af.text_field :latitude, { placeholder: t(:latitude_placeholder) }
-  .four.columns.omega
-    = af.text_field :longitude, { placeholder: t(:longitude_placeholder) }
-.row
-  .three.columns.alpha
-    = " ".html_safe
-  .five.columns.omega
-    = check_box_tag "use_geocoder"
-    = label_tag "use_geocoder", t('use_geocoder')
+    )
+  .row
+    .three.columns.alpha
+      = af.label :address1, t(:address)
+      %span.required *
+    .eight.columns.omega
+      = af.text_field :address1, { placeholder: t(:address_placeholder) }
+  .row
+    .alpha.three.columns
+      = af.label :address2, t(:address2)
+    .eight.columns.omega
+      = af.text_field :address2
+  .row
+    .three.columns.alpha
+      = af.label :city, t(:city)
+      \/
+      = af.label :zipcode, t(:postcode)
+      %span.required *
+    .four.columns
+      = af.text_field :city, { placeholder: t(:city_placeholder) }
+    .four.columns.omega
+      = af.text_field :zipcode, { placeholder: t(:postcode_placeholder) }
+  .row{"data-controller": "dependent-select", "data-dependent-select-options-value": countries_with_states }
+    .three.columns.alpha
+      = af.label :state_id, t(:state)
+      \/
+      = af.label :country_id, t(:country)
+      %span.required *
+    .four.columns
+      = af.select :state_id, @enterprise.address.country.states.map { |s| [s.name, s.id] }, {}, { "data-controller": "tom-select", "data-dependent-select-target": "select", class: "primary" }
+    .four.columns.omega{ data: { controller: "primary-details" }}
+      = af.select :country_id, available_countries.map { |c| [c.name, c.id] }, {}, { "data-controller": "tom-select", "data-dependent-select-target": "source", "data-action": "dependent-select#handleSelectChange", class: "primary" }
+  .row
+    .three.columns.alpha
+      = af.label :latitude, t(:latitude)
+      \/
+      = af.label :longitude, t(:longitude)
+      %span.required *
+      = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('latitude_longitude_tip')}
+    .four.columns
+      = af.text_field :latitude, { placeholder: t(:latitude_placeholder) }
+    .four.columns.omega
+      = af.text_field :longitude, { placeholder: t(:longitude_placeholder) }
+  .row
+    .three.columns.alpha
+      = " ".html_safe
+    .five.columns.omega
+      = check_box_tag "use_geocoder"
+      = label_tag "use_geocoder", t('use_geocoder')

app/views/admin/reports/_rendering_options.html.haml

@@ -16,16 +16,16 @@
       .omega.fourteen.columns
         - if @report.header_option?
           %span.inline-checkbox{ style: "margin-right: 1rem;" }
-            = check_box_tag :display_header_row, true, params[:display_header_row]
+            = check_box_tag :display_header_row, true, @render_options.options[:display_header_row]
             = label_tag :display_header_row, t(".header_row")
         - if @report.summary_row_option?
           %span.inline-checkbox
-            = check_box_tag :display_summary_row, true, params[:display_summary_row], { "data-csv-select-target": "checkbox" }
+            = check_box_tag :display_summary_row, true, @render_options.options[:display_summary_row], { "data-csv-select-target": "checkbox" }
             = label_tag :display_summary_row, t(".summary_row"), { "data-csv-select-target": "label" }
 
 - if @report.available_headers.present?
   .row
     .alpha.two.columns= label_tag nil, t(:report_columns)
     .omega.fourteen.columns
-      = render MultipleCheckedSelectComponent.new(name: "fields_to_show", options: @report.available_headers, selected: @params_fields_to_show)
+      = render MultipleCheckedSelectComponent.new(name: "fields_to_show", options: @report.available_headers, selected: @render_options.options[:fields_to_show])
   

app/webpacker/css/admin/openfoodnetwork.scss

@@ -195,7 +195,7 @@ table#listing_enterprise_groups {
 }
 
 .fullwidth_inputs {
-  input[type=text], select {
+  #{$text-inputs}, select {
     width: 100%;
   }
 }

app/webpacker/css/admin/shared/forms.scss

@@ -1,12 +1,11 @@
-input[type="text"],
-input[type="password"],
-input[type="email"],
+$text-inputs:
+  "input[type=text], input[type=password], input[type=email], input[type=url], input[type=tel]";
+
+#{$text-inputs},
 input[type="date"],
 input[type="datetime"],
 input[type="time"],
-input[type="url"],
 input[type="number"],
-input[type="tel"],
 textarea, fieldset {
   @include border-radius($border-radius);
   padding: 7px 10px;

config/locales/en.yml

@@ -706,6 +706,7 @@ en:
         conditional_blank: can't be blank if unit_type is blank
         no_product: did not match any products in the database
         not_found: not found in database
+        category_not_found: doesn't match allowed categories. See the correct categories to choose from on the product import page, or check that there's no misspelling.
         not_updatable: cannot be updated on existing products via product import
         blank: can't be blank
         products_no_permission: you do not have permission to manage products for this enterprise

db/migrate/20221116095826_create_report_rendering_options.rb

@@ -0,0 +1,12 @@
+class CreateReportRenderingOptions < ActiveRecord::Migration[6.1]
+  def change
+    create_table :report_rendering_options do |t|
+      t.references :user
+      t.text :options
+      t.string :report_type
+      t.string :report_subtype
+
+      t.timestamps
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_10_07_105052) do
+ActiveRecord::Schema.define(version: 2022_11_16_095826) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_stat_statements"
@@ -354,6 +354,16 @@ ActiveRecord::Schema.define(version: 2022_10_07_105052) do
     t.index ["subscription_id"], name: "index_proxy_orders_on_subscription_id"
   end
 
+  create_table "report_rendering_options", force: :cascade do |t|
+    t.bigint "user_id"
+    t.text "options"
+    t.string "report_type"
+    t.string "report_subtype"
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["user_id"], name: "index_report_rendering_options_on_user_id"
+  end
+
   create_table "schedules", id: :serial, force: :cascade do |t|
     t.string "name", limit: 255, null: false
     t.datetime "created_at", null: false

spec/models/product_importer_spec.rb

@@ -272,6 +272,26 @@ describe ProductImport::ProductImporter do
     end
   end
 
+  describe "when shipping category is not found" do
+    let(:csv_data) {
+      CSV.generate do |csv|
+        csv << ["name", "producer", "category", "on_hand", "price", "units", "unit_type",
+                "variant_unit_name", "on_demand", "shipping_category"]
+        csv << ["Shipping Test", enterprise.name, "Vegetables", "5", "3.20", "500", "g", "", nil,
+                "not_found"]
+      end
+    }
+    let(:importer) { import_data csv_data }
+
+    it "raises an error" do
+      importer.validate_entries
+      entries = JSON.parse(importer.entries_json)
+      error = entries['2']['errors']['shipping_category']
+
+      expect(error).to include "Shipping_category doesn't match allowed categories"
+    end
+  end
+
   describe "when enterprises are not valid" do
     let(:csv_data) {
       CSV.generate do |csv|

spec/models/report_rendering_options_spec.rb

@@ -0,0 +1,5 @@
+require 'rails_helper'
+
+RSpec.describe ReportRenderingOptions, type: :model do
+  pending "add some examples to (or delete) #{__FILE__}"
+end

spec/system/admin/bulk_order_management_spec.rb

@@ -380,7 +380,7 @@ describe '
           visit_bulk_order_management
         end
 
-        it "displays a select box for distributors, which filters line items by the selected distributor", retry: 2 do
+        it "displays a select box for distributors, which filters line items by the selected distributor", retry: 3 do
           expect(page).to have_selector "tr#li_#{li1.id}"
           expect(page).to have_selector "tr#li_#{li2.id}"
           find("#s2id_distributor_filter .select2-chosen").click
@@ -393,7 +393,7 @@ describe '
           expect(page).to have_no_selector "tr#li_#{li2.id}"
         end
 
-        it "displays all line items when 'All' is selected from distributor filter", retry: 2 do
+        it "displays all line items when 'All' is selected from distributor filter", retry: 3 do
           # displays orders from one enterprise only
           expect(page).to have_selector "tr#li_#{li2.id}"
           find("#s2id_distributor_filter .select2-chosen").click

spec/system/admin/configuration/taxonomies_spec.rb

@@ -43,10 +43,6 @@ describe "Taxonomies" do
   end
 
   context "edit" do
-    before do
-      Capybara.raise_server_errors = false
-    end
-
     it "should allow an admin to update an existing taxonomy" do
       create(:taxonomy)
       click_link "Taxonomies"

vendor/assets/javascripts/jquery.jstree/themes/apple/style.css

@@ -17,7 +17,6 @@
 .jstree-apple .jstree-hovered { background:#e7f4f9; border:1px solid #d8f0fa; padding:0 3px 0 1px; text-shadow:1px 1px 1px silver; }
 .jstree-apple .jstree-clicked { background:#beebff; border:1px solid #99defd; padding:0 3px 0 1px; }
 .jstree-apple a .jstree-icon { background-position:-56px -20px; }
-.jstree-apple a.jstree-loading .jstree-icon { background:url("throbber.gif") center center no-repeat !important; }
 
 .jstree-apple.jstree-focused { background:white; }