From d14700c51d692335f001a93c2f6b13b135783206 Mon Sep 17 00:00:00 2001 From: IN COMMON Collective Date: Thu, 8 Apr 2021 16:34:15 +0200 Subject: [FIX] Use form model to create/edit resources (fixes #4, fixes #5, refs #3) Since we must associate other models (e.g., classifications) to a Resource, we use a composite model to save all changes inside a database transaction. This approach makes it simpler to handle resources and their associations. Work remains to fix the geolocation and reverse geolocation to ensure these are in sync. --- app/controllers/resources_controller.rb | 31 ++++--- app/helpers/resources_helper.rb | 5 +- app/models/classification.rb | 2 + app/models/resource.rb | 11 +-- app/models/resource_form.rb | 138 ++++++++++++++++++++++++++++++++ app/views/resources/_edit.html.erb | 4 +- app/views/resources/_form.html.erb | 37 ++++++--- app/views/resources/new.html.erb | 4 +- config/routes.rb | 1 + 9 files changed, 194 insertions(+), 39 deletions(-) create mode 100644 app/models/resource_form.rb diff --git a/app/controllers/resources_controller.rb b/app/controllers/resources_controller.rb index 2fa392f..4a99f2a 100644 --- a/app/controllers/resources_controller.rb +++ b/app/controllers/resources_controller.rb @@ -3,7 +3,8 @@ # SPDX-License-Identifier: AGPL-3.0-or-later class ResourcesController < ApplicationController - before_action :set_resource, only: [:new, :show, :edit, :update, :delete, :destroy] + before_action :set_resource, only: [:show, :delete, :destroy] +# before_action :set_resource_form, only: [:new, :create, :edit, :update] # GET /resources def index @@ -12,6 +13,7 @@ class ResourcesController < ApplicationController # GET /resources/new def new + @resource = ResourceForm.new(current_agent) end # POST /resources @@ -19,18 +21,12 @@ class ResourcesController < ApplicationController # TODO Background job to list similar items # TODO If there's a match, return to user with new record or list of mergeable ones - classification = resource_params.delete(:classification) || { section_ids: [] } - - Rails.logger.info resource_params - - @resource = current_agent.resources.build(resource_params) + @resource = ResourceForm.new(current_agent, resource_form_params) respond_to do |format| - Rails.logger.info "format: #{format} - Res: #{@resource.inspect}" if @resource.save - classification[:section_ids].each { |id| @resource.classifications.find_or_create_by(section_id: id) } - format.html { redirect_to @resource, notice: 'Merci de votre contribution !' } - format.json { render :show, status: :created, location: @resource } + format.html { redirect_to @resource.resource, notice: 'Merci de votre contribution !' } + format.json { render :show, status: :created, location: @resource.resource } else format.html { render :new } format.json { render json: @resource.errors, status: :unprocessable_entity } @@ -62,8 +58,8 @@ class ResourcesController < ApplicationController respond_to do |format| if @resource.update(resource_params) - format.html { redirect_to @resource, notice: 'Merci de votre contribution !' } - format.json { render :show, status: :ok, location: @resource } + format.html { redirect_to @resource.resource, notice: 'Merci de votre contribution !' } + format.json { render :show, status: :ok, location: @resource.resource } else format.html { render :edit } format.json { render json: @resource.errors, status: :unprocessable_entity } @@ -97,9 +93,9 @@ class ResourcesController < ApplicationController private - def resource_params + def resource_form_params params - .require(:resource) + .require(:resource_form) .permit(:agent_id, :uuid, :name, @@ -111,11 +107,12 @@ class ResourcesController < ApplicationController :address, :postal_code, :city, - :entry_number, - :categories, + :entry_number, # Unused on new resources + :categories, # :latitude, :longitude, - classification: [ :section_ids ]) + :source, + section_ids: []) end def set_resource diff --git a/app/helpers/resources_helper.rb b/app/helpers/resources_helper.rb index a949311..792a78a 100644 --- a/app/helpers/resources_helper.rb +++ b/app/helpers/resources_helper.rb @@ -4,13 +4,14 @@ module ResourcesHelper # Return a SELECT tag to choose a section in a given taxonomy - def section_select(taxonomy, selected = nil) + def section_select(taxonomy, selected = nil, options = {}) grouped_options = [] + tag_name = options[:name] || 'classification[section_ids]' taxonomy.categories.each do |c| grouped_options << [c.name, c.sections.map { |s| [s.name, s.id] }] end - select_tag('classification[section_ids]', grouped_options_for_select(grouped_options), selected: selected, multiple: true) + select_tag(tag_name, grouped_options_for_select(grouped_options, selected), multiple: true) end end diff --git a/app/models/classification.rb b/app/models/classification.rb index 0754054..e2b180f 100644 --- a/app/models/classification.rb +++ b/app/models/classification.rb @@ -5,4 +5,6 @@ class Classification < ApplicationRecord belongs_to :resource belongs_to :section + + self.primary_key = :resource_id end diff --git a/app/models/resource.rb b/app/models/resource.rb index c603978..b803afb 100644 --- a/app/models/resource.rb +++ b/app/models/resource.rb @@ -21,7 +21,7 @@ class Resource < ApplicationRecord length: { in: 3..64 } validates :email, - with: { format: URI::MailTo::EMAIL_REGEXP }, + format: { with: URI::MailTo::EMAIL_REGEXP }, allow_blank: true validates :source, @@ -31,7 +31,8 @@ class Resource < ApplicationRecord phony_normalize :phone_number, default_country_code: 'BE', normalize_when_valid: true validates :phone_number, - phony_plausible: { ignore_record_country_code: true, ignore_record_country_number: true } + phony_plausible: { ignore_record_country_code: true, ignore_record_country_number: true }, + allow_blank: true # Depends on validate_url Gem validates :website, @@ -47,19 +48,19 @@ class Resource < ApplicationRecord format('%3.7f', lon: feature['geometry']['coordinates'][0]).to_f end def longitude=(value) - feature['geometry']['coordinates'][0] = format('%3.7f', lon: value).to_f + feature['geometry']['coordinates'][0] = format('%3.7f', lon: value.to_f).to_f end # You can use, e.g.: res.latitude = 0.123 def latitude format('%2.7f', lat: feature['geometry']['coordinates'][1]).to_f end def latitude=(value) - feature['geometry']['coordinates'][1] = format('%2.7f', lat: value).to_f + feature['geometry']['coordinates'][1] = format('%2.7f', lat: value.to_f).to_f end # Properties - [:name, :summary, :description, :email, :source, :address, :postal_code, :city, :phone_number, :website].each do |prop| + [:name, :summary, :description, :email, :source, :address, :postal_code, :city, :phone_number, :website, :entry_number].each do |prop| # Define a reader define_method prop do properties[prop.to_s] diff --git a/app/models/resource_form.rb b/app/models/resource_form.rb new file mode 100644 index 0000000..14275a4 --- /dev/null +++ b/app/models/resource_form.rb @@ -0,0 +1,138 @@ +class ResourceForm + include ActiveModel::Model + + attr_accessor :agent, :resource + attr_accessor :agent_id, :uuid, :name, :summary, :description, :email, + :website, :phone_number, :address, :postal_code, :city, + :entry_number, :latitude, :longitude, :section_ids, :source + + with_options presence: true do + validates :name + validates :email, allow_blank: true +# validates :source + validates :phone_number, allow_blank: true + validates :website, allow_blank: true + end + + validate :validate_models + + def initialize(agent, attributes = nil) + @agent = agent + @attributes = attributes || default_attributes + + raise ArgumentError "Attributes must be permitted" unless @attributes.permitted? + + # We use an empty hidden field to ensure this is always set, even if no + # section is selected, so let's remove the empty value + @section_ids = @attributes.extract!(:section_ids)[:section_ids].filter_map { |x| x.presence } rescue [] + # We may have an existing and correct source, but anyway we must have one + @resource = @agent.resources.build({ source: @agent.name }.merge(@attributes)) + end + + def new_record? + resource&.new_record? || true + end + + # New forms can provide an existing classification, + # and updates can prefer passed arguments to ensure correct changes + def section_ids + resource.present? && @section_ids.empty? ? resource.section_ids : @section_ids + end + + # Ensure field values stick around + # OMG this is ugly + # TODO: use delegator or metaprog + def name + @name ||= resource&.name + end + def summary + @summary ||= resource&.summary + end + def description + @description ||= resource&.description + end + def email + @email ||= resource&.email + end + def website + @website ||= resource&.website + end + def phone_number + @phone_number ||= resource&.phone_number + end + def address + @address ||= resource&.address + end + def postal_code + @postal_code ||= resource&.postal_code + end + def city + @city ||= resource&.city + end + def longitude + @longitude ||= resource&.longitude + end + def latitude + @latitude ||= resource&.latitude + end + + + def save + Rails.logger.info "--- Calling SAVE ---" + return false if invalid? + + ActiveRecord::Base.transaction do + resource.save! && \ + @section_ids.each do |sec_id| + Rails.logger.info("section " + sec_id) + resource.categories << resource.classifications.find_or_create_by!(section_id: sec_id) + end + + true + rescue ActiveRecord::StatementInvalid => e + errors.add(:base, e.message) + false + end + + rescue Exception => e + Rails.logger.info "Unhandled Exception #{e.class}: #{e.message}" + false + end + + private + + def default_attributes + ActionController::Parameters.new( + { + agent_id: @agent.id, + uuid: '', + name: '', + summary: '', + description: '', + email: '', + website: '', + phone_number: '', + address: '', + postal_code: '', + city: '', + entry_number: nil, + latitude: 0.0, + longitude: 0.0, + source: @agent.name, + section_ids: [] + }).permit! + end + + # Validate underlying models + def validate_models + Rails.logger.info('--- validate_models ---') + resource.valid? || promote_errors(resource.errors) + end + + # Promote an error from the Model to the Form + def promote_errors(model_errors) + model_errors.each do |attribute, error| + errors.add(attribute, error.full_message) + end + end +end diff --git a/app/views/resources/_edit.html.erb b/app/views/resources/_edit.html.erb index 55c59f6..f22aee3 100644 --- a/app/views/resources/_edit.html.erb +++ b/app/views/resources/_edit.html.erb @@ -1,6 +1,6 @@ -

Edit <%= @resource.presence.feature['properties']['name'] || 'new resource' %>

+

Edit <%= @resource.presence.name || 'new resource' %>

Context: <%= current_agent %>

-<%= form_with model: [current_agent,@resource], url: controller.action_name == 'new' ? agent_resources_path(agent: current_agent) : resource_path(@resource) do |f| %> +<%= form_with model: @resource, url: @resource.presence.new_record? ? agent_resources_path(current_agent) : resource_path(@resource.resource) do |f| %> <%= render partial: 'form', locals: { resource: resource, f: f } %> <% end %> diff --git a/app/views/resources/_form.html.erb b/app/views/resources/_form.html.erb index 89745bd..648834d 100644 --- a/app/views/resources/_form.html.erb +++ b/app/views/resources/_form.html.erb @@ -1,12 +1,24 @@ -
+<% if resource.errors.any? %> +
+

<%= pluralize(resource.errors.count, "error") %> prohibited this resource from being saved:

+ +
    + <% resource.errors.each do |error| %> +
  • <%= error.full_message %>
    • + <% end %> +
+
+<% end %> + +
<%= tag.legend "Propriétés de la ressource" %> -
+
<%= f.label :name %>
-
<%= f.text_field :name, maxlength: 64, placeholder: 'La ferme des animaux', value: resource.name %>
+
<%= f.text_field :name, maxlength: 64, placeholder: 'La ferme des animaux' %>
<%= f.label :summary %>
-
<%= f.text_field :summary, maxlength: 136, placeholder: 'Une ferme locale', value: resource.name %>
+
<%= f.text_field :summary, maxlength: 136, placeholder: 'Une ferme locale' %>
<%= f.label :description %>
<%= f.text_area :description, cols: 72, rows: 10, placeholder: '## Un choix pertinent @@ -29,33 +41,34 @@ La description _peut_ comporter du [Markdown].
<%= tag.legend "Classification" %> <%# TODO Add a taxonomy selector %> - <%= section_select(current_agent.taxonomies&.first || Taxonomy.first) %> + <%= section_select(current_agent.taxonomies&.first || Taxonomy.first, resource.section_ids, name: 'resource_form[section_ids]') %>
<%= tag.legend "Coordonées géographiques" %> <%# TODO Add a graphical geo selector %> -
+
<%= f.label :address %>
-
<%= f.text_field :address, value: resource.address, placeholder: 'Rue aux herbes, 123' %>
+
<%= f.text_field :address, placeholder: 'Rue aux herbes, 123' %>
<%= f.label :postal_code %>
-
<%= f.text_field :postal_code, value: resource.postal_code, placeholder: '1000' %>
+
<%= f.text_field :postal_code, placeholder: '1000' %>
<%= f.label :city %>
-
<%= f.text_field :city, value: resource.city, placeholder: 'Bruxelles' %>
+
<%= f.text_field :city, placeholder: 'Bruxelles' %>
-
+
<%= f.label :longitude %>
-
<%= f.text_field :longitude, value: resource.longitude, placeholder: '0.56789012' %>
+
<%= f.text_field :longitude, placeholder: '0.56789012', pattern: '[\d]+.[\d]+' %>
<%= f.label :latitude %>
-
<%= f.text_field :latitude, value: resource.latitude, placeholder: '50.12345678' %>
+
<%= f.text_field :latitude, placeholder: '50.12345678', pattern: '[\d]+.[\d]+' %>
<%= f.hidden_field :agent_id, value: current_agent.id %> +<%= f.hidden_field :source, value: current_agent.name %>

<%= f.submit 'Save' %>

diff --git a/app/views/resources/new.html.erb b/app/views/resources/new.html.erb index 80d8206..e8347b4 100644 --- a/app/views/resources/new.html.erb +++ b/app/views/resources/new.html.erb @@ -1,2 +1,4 @@ -<%= render partial: 'edit', locals: { resource: @resource } %> +<%= render 'edit', resource: @resource %> + +<#%= render partial: 'edit', locals: { resource: @resource } %> diff --git a/config/routes.rb b/config/routes.rb index 54f383e..7fd064a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -2,6 +2,7 @@ # SPDX-License-Identifier: AGPL-3.0-or-later Rails.application.routes.draw do + get 'sections/show' # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html -- cgit v1.2.3