Skip to content
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.

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

Feature: Display additional fields when attaching a record to another #3048

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
670ba0a
add fields
binarygit Jul 30, 2024
f79177e
Apply options like update_using when attaching record
binarygit Jul 31, 2024
689d72a
Lint
binarygit Jul 31, 2024
c7ae36c
Use keyword arguments
binarygit Jul 31, 2024
fd1f441
Ensure through attachments without additional params are correctly at…
binarygit Jul 31, 2024
f6cfb9e
Refactor: use fill_record method itself
binarygit Jul 31, 2024
a60de9f
Refactor: do not send redundant args to fieldsexecContext
binarygit Jul 31, 2024
abdd39d
Add translation key
binarygit Jul 31, 2024
37f9c60
Refactor: use @reflection and remove duplication
binarygit Jul 31, 2024
b6b0e85
Use extra_fields instead of extra
binarygit Jul 31, 2024
31dd758
Update app/controllers/avo/associations_controller.rb
binarygit Jul 31, 2024
77e2988
Merge branch 'main' into display-additional-columns-in-join-records
Paul-Bob Jul 31, 2024
cfd4090
wip styling
adrianthedev Aug 1, 2024
ec2f378
include blank on the select
binarygit Aug 1, 2024
109e0f7
Do not pass in nil value for model because it is deprecated
binarygit Aug 1, 2024
30f37cb
Remove duplicate key
binarygit Aug 1, 2024
950a88e
Format
binarygit Aug 1, 2024
78bbb18
Give clear name
binarygit Aug 1, 2024
7e909a1
Update app/controllers/avo/associations_controller.rb
binarygit Aug 1, 2024
73d8829
Rename to attach_fields
binarygit Aug 1, 2024
1993546
Merge branch 'main' into display-additional-columns-in-join-records
Paul-Bob Aug 2, 2024
1a1ec9e
Merge branch 'main' into display-additional-columns-in-join-records
Paul-Bob Aug 2, 2024
6751d8a
Display correct label
binarygit Aug 5, 2024
a748541
Enable additional fields when select is searchable
binarygit Aug 5, 2024
8d057bf
Lint
binarygit Aug 5, 2024
4412710
Add label_text
binarygit Aug 5, 2024
edc6f5a
Merge branch 'main' into display-additional-columns-in-join-records
Paul-Bob Aug 5, 2024
76a01ff
Fix indentation
binarygit Aug 5, 2024
90a64f9
Change texts to reflect the association
binarygit Aug 5, 2024
19d5da0
Fix tests
binarygit Aug 5, 2024
c2aed1b
Fix feature test
binarygit Aug 5, 2024
76a7cc2
Refactor associations controller and display sigularized field names
binarygit Aug 5, 2024
12c96e8
Remove useless assignment
binarygit Aug 5, 2024
981ecee
Update app/controllers/avo/associations_controller.rb
Paul-Bob Aug 5, 2024
c1a463a
Update app/controllers/avo/associations_controller.rb
Paul-Bob Aug 5, 2024
a40e604
Update app/views/avo/associations/new.html.erb
Paul-Bob Aug 5, 2024
83ffa20
Update app/views/avo/associations/new.html.erb
Paul-Bob Aug 5, 2024
0a8a670
Update spec/system/avo/for_attribute_spec.rb
Paul-Bob Aug 5, 2024
8ce929d
Update app/controllers/avo/associations_controller.rb
Paul-Bob Aug 5, 2024
4763aac
field_wrapper on searchable
Paul-Bob Aug 5, 2024
e16142b
Merge branch 'main' into display-additional-columns-in-join-records
Paul-Bob Aug 5, 2024
a00b433
Update app/controllers/avo/associations_controller.rb
Paul-Bob Aug 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion app/controllers/avo/associations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class AssociationsController < BaseController
before_action :set_attachment_class, only: [:show, :index, :new, :create, :destroy]
before_action :set_attachment_resource, only: [:show, :index, :new, :create, :destroy]
before_action :set_attachment_record, only: [:create, :destroy]
before_action :set_extra_fields, only: [:new, :create]
before_action :authorize_index_action, only: :index
before_action :authorize_attach_action, only: :new
before_action :authorize_detach_action, only: :destroy
Expand Down Expand Up @@ -67,6 +68,7 @@ def create
notice: t("avo.attachment_class_attached", attachment_class: @related_resource.name)
}
else
flash[:error] = t("avo.attachment_failed", attachment_class: @related_resource.name)
format.turbo_stream {
render turbo_stream: turbo_stream.append("alerts", partial: "avo/partials/all_alerts")
}
Expand All @@ -78,7 +80,13 @@ def create_association
association_name = BaseResource.valid_association_name(@record, association_from_params)

perform_action_and_record_errors do
if has_many_reflection?
if through_reflection?
if additional_params?
new_join_record.save
else
@record.send(association_name) << @attachment_record
end
elsif has_many_reflection?
binarygit marked this conversation as resolved.
Show resolved Hide resolved
binarygit marked this conversation as resolved.
Show resolved Hide resolved
@record.send(association_name) << @attachment_record
else
@record.send(:"#{association_name}=", @attachment_record)
Expand Down Expand Up @@ -194,5 +202,40 @@ def has_many_reflection?
ActiveRecord::Reflection::HasAndBelongsToManyReflection
]
end

def through_reflection?
reflection.instance_of? ActiveRecord::Reflection::ThroughReflection
end

def additional_params?
additional_params.keys.count >= 1
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def additional_params?
additional_params.keys.count >= 1
end

Use additional_params.any? instead of dedicated method


def additional_params
params[:fields].except("related_id")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ok? Taking all the params that the user gives us?
It feels like a security issue. A bad actor can add fields in the DOM.

Can we just allow the fields that we need and know that the user added from the field DSL?

end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def additional_params
params[:fields].except("related_id")
end
def additional_params
params[:fields].permit(@attach_fields.map(&:id))
end


def set_extra_fields
@extra = if @field.extra.present?
Avo::FieldsExecutionContext.new(target: @field.extra, parent: @record)
.detect_fields
.items_holder
.items
end
end

def new_join_record
field_names = Avo::FieldsExecutionContext.new(target: @field.extra, parent: @resource)
binarygit marked this conversation as resolved.
Show resolved Hide resolved
.detect_fields
.items
.map { |field| field.name.tr(" ", "_").downcase }

@resource.fill_join_record(
record: reflection.through_reflection.klass.new,
fields: @extra,
params: additional_params.permit(field_names).merge({source_foreign_key => @attachment_record.id, through_foreign_key => @record.id}),
extra_params: [source_foreign_key, through_foreign_key]
)
end
end
end
3 changes: 3 additions & 0 deletions app/views/avo/associations/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
class: input_classes('w-full'),
}
%>
<% @extra&.each_with_index do |ex, index| %>
<%= render(Avo::Items::SwitcherComponent.new(resource: @related_resource, item: ex, index: index + 1, view: @view, form: form, field_component_extra_args: {stacked: true})) %>
binarygit marked this conversation as resolved.
Show resolved Hide resolved
<% end %>
</div>
<% end %>
</div>
Expand Down
2 changes: 2 additions & 0 deletions lib/avo/fields/has_base_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class HasBaseField < BaseField
attr_accessor :discreet_pagination
attr_accessor :hide_search_input
attr_reader :link_to_child_resource
attr_reader :extra

def initialize(id, **args, &block)
super(id, **args, &block)
Expand All @@ -27,6 +28,7 @@ def initialize(id, **args, &block)
@link_to_child_resource = args[:link_to_child_resource] || false
@reloadable = args[:reloadable].present? ? args[:reloadable] : false
@linkable = args[:linkable].present? ? args[:linkable] : false
@extra = args[:extra].present? ? args[:extra] : nil
binarygit marked this conversation as resolved.
Show resolved Hide resolved
end

def field_resource
Expand Down
13 changes: 13 additions & 0 deletions lib/avo/fields_execution_context.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module Avo
class FieldsExecutionContext < Avo::ExecutionContext
include Avo::Concerns::HasItems

def detect_fields
self.items_holder = Avo::Resources::Items::Holder.new(parent: self)

instance_exec(&target) if target.present? && target.respond_to?(:call)

self
end
end
end
51 changes: 36 additions & 15 deletions lib/avo/resources/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -285,14 +285,16 @@ def detect_fields
self
end

VIEW_METHODS_MAPPING = {
index: [:index_fields, :display_fields],
show: [:show_fields, :display_fields],
edit: [:edit_fields, :form_fields],
update: [:edit_fields, :form_fields],
new: [:new_fields, :form_fields],
create: [:new_fields, :form_fields]
} unless defined? VIEW_METHODS_MAPPING
unless defined? VIEW_METHODS_MAPPING
VIEW_METHODS_MAPPING = {
index: [:index_fields, :display_fields],
show: [:show_fields, :display_fields],
edit: [:edit_fields, :form_fields],
update: [:edit_fields, :form_fields],
new: [:new_fields, :form_fields],
create: [:new_fields, :form_fields]
}
end

def fetch_fields
possible_methods_for_view = VIEW_METHODS_MAPPING[view.to_sym]
Expand Down Expand Up @@ -334,12 +336,12 @@ def divider(label = nil)
end

# def get_actions / def get_filters / def get_scopes
define_method "get_#{plural_entity}" do
define_method :"get_#{plural_entity}" do
return entity_loader(entity).bag if entity_loader(entity).present?

# ex: @actions_loader = Avo::Loaders::ActionsLoader.new
instance_variable_set(
"@#{plural_entity}_loader",
:"@#{plural_entity}_loader",
"Avo::Loaders::#{plural_entity.humanize}Loader".constantize.new
)

Expand All @@ -349,8 +351,8 @@ def divider(label = nil)
end

# def get_action_arguments / def get_filter_arguments / def get_scope_arguments
define_method "get_#{entity}_arguments" do |entity_class|
klass = send("get_#{plural_entity}").find { |entity| entity[:class].to_s == entity_class.to_s }
define_method :"get_#{entity}_arguments" do |entity_class|
klass = send(:"get_#{plural_entity}").find { |entity| entity[:class].to_s == entity_class.to_s }

raise "Couldn't find '#{entity_class}' in the 'def #{plural_entity}' method on your '#{self.class}' resource." if klass.nil?

Expand All @@ -359,7 +361,7 @@ def divider(label = nil)
end

def hydrate(...)
super(...)
super
binarygit marked this conversation as resolved.
Show resolved Hide resolved
binarygit marked this conversation as resolved.
Show resolved Hide resolved

if @record.present?
hydrate_model_with_default_values if @view&.new?
Expand Down Expand Up @@ -395,7 +397,7 @@ def record_title
return name if @record.nil?

# Get the title from the record if title is not set, try to get the name, title or label, or fallback to the id
return @record.try(:name) || @record.try(:title) || @record.try(:label) || @record.id if title.nil?
return @record.try(:name) || @record.try(:title) || @record.try(:label) || @record.id if title.nil?

# If the title is a symbol, get the value from the record else execute the block/string
case title
Expand Down Expand Up @@ -462,6 +464,25 @@ def fill_record(record, params, extra_params: [])
record
end

def fill_join_record(record:, fields:, params:, extra_params:)
# Write the field values
params.each do |key, value|
field = fields.find { |f| f.id == key.to_sym }

next unless field.present?

record = field.fill_field record, key, value, params
end

# Write the user configured extra params to the record
if extra_params.present?
# Let Rails fill in the rest of the params
record.assign_attributes params.permit(extra_params)
end

record
end
binarygit marked this conversation as resolved.
Show resolved Hide resolved

def authorization(user: nil)
current_user = user || Avo::Current.user
Avo::Services::AuthorizationService.new(current_user, record || model_class, policy_class: authorization_policy)
Expand Down Expand Up @@ -601,7 +622,7 @@ def description_attributes
end

def entity_loader(entity)
instance_variable_get("@#{entity.to_s.pluralize}_loader")
instance_variable_get(:"@#{entity.to_s.pluralize}_loader")
end
end
end
Expand Down
11 changes: 11 additions & 0 deletions spec/dummy/app/avo/resources/store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,16 @@ def fields
# Example for error message when resource is missing
field :location, as: :has_one
end

field :patrons, as: :has_many, through: :patronships,
translation_key: "patrons",
extra: -> {
if ENV["TEST_FILL_JOIN_RECORD"]
field :review, as: :text,
update_using: -> { ">> #{value} <<" }
else
field :review, as: :text
end
}
end
end
3 changes: 3 additions & 0 deletions spec/dummy/app/models/store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@
#
class Store < ApplicationRecord
has_one :location

has_many :patronships, class_name: :StorePatron
has_many :patrons, through: :patronships, class_name: :User, source: :user
end
6 changes: 6 additions & 0 deletions spec/dummy/app/models/store_patron.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class StorePatron < ApplicationRecord
belongs_to :store
belongs_to :user

validates :review, presence: true
end
1 change: 1 addition & 0 deletions spec/dummy/config/locales/avo.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ en:
attach_item: Attach %{item}
attachment_class_attached: "%{attachment_class} attached."
attachment_class_detached: "%{attachment_class} detached."
attachment_failed: "Failed to attach %{attachment_class}"
attachment_destroyed: Attachment destroyed
cancel: Cancel
choose_a_country: Choose a country
Expand Down
11 changes: 11 additions & 0 deletions spec/dummy/db/migrate/20240724090242_create_store_patrons.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class CreateStorePatrons < ActiveRecord::Migration[6.1]
def change
create_table :store_patrons do |t|
t.integer :store_id
t.integer :user_id
t.string :review

t.timestamps
end
end
end
10 changes: 9 additions & 1 deletion spec/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "price_cents", default: 0, null: false
t.string "price_currency", default: "'USD'::character varying", null: false
t.string "price_currency", default: "USD", null: false
end

create_table "projects", force: :cascade do |t|
Expand Down Expand Up @@ -208,6 +208,14 @@
t.index ["user_id"], name: "index_reviews_on_user_id"
end

create_table "store_patrons", force: :cascade do |t|
t.integer "store_id"
t.integer "user_id"
t.string "review"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
end

create_table "stores", force: :cascade do |t|
t.string "name"
t.string "size"
Expand Down
46 changes: 46 additions & 0 deletions spec/system/avo/attach_with_additional_fields_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# frozen_string_literal: true

require "rails_helper"

RSpec.describe "Attach with extra fields", type: :system do
let!(:store) { create(:store) }
let!(:user) { create :user }
let!(:url) { "/admin/resources/stores/#{store.id}/patrons/new?view=show" }

it "allows to pass in data for extra fields" do
visit url
expect(page).to have_selector "input#fields_review"
expect(page).to have_selector "select#fields_related_id"

select user.name

expect {
click_button "Attach"
}.not_to change(StorePatron, :count)

expect(page).to_not have_text "Failed to attach User"

select user.name
fill_in id: "fields_review", with: "Toilet paper is phenomenal here."

expect {
click_button "Attach"
}.to change(StorePatron, :count).by 1
expect(page).to have_text "User attached"
end

it "saves attachment adhering to options like update_using" do
ENV["TEST_FILL_JOIN_RECORD"] = "1"
visit url
expect(page).to have_selector "input#fields_review"
expect(page).to have_selector "select#fields_related_id"

select user.name
fill_in id: "fields_review", with: "Toilet paper is phenomenal here."

expect {
click_button "Attach"
}.to change(StorePatron, :count)
expect(store.patronships.first.review).to eq ">> Toilet paper is phenomenal here. <<"
end
end
Loading