Feature: Display additional fields when attaching a record to another

[binarygit]

Description

Fixes #2472

Documentation PR: avo-hq/docs.avohq.io#255

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

When attaching fails:

error.mp4

When attaching is successful:

fixed.mp4

Manual review steps

1. Step 1
2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

[codeclimate]

Code Climate has analyzed commit a00b433 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

[Paul-Bob]

There are some conflicts (I guess from this PR) and i18n test is failing.

Let's fix those first please

When fixing the conflicts let's keep the turbo stream response and incorporate the t("avo.attachment_failed", attachment_class: @related_resource.name) in the errors list.

[binarygit]

@Paul-Bob I can fix the i18n tests before we merge this. It's complaining about missing keys in other i18n files. Currently I will need to go into every one of them and put the key in and we might change the words or something so it might get repetitive.

Could you please review this and I'll make sure I add those keys in before we merge.

[Paul-Bob]

.

[Paul-Bob]

It's looking good! Nice progress @binarygit!

Let's try to use the fields to parse the values that come through params.

[Paul-Bob]

Hey @binarygit we're almost there!

I left some comments, let's prioritize the fill_record method duplication.

[binarygit]

@adrianthedev We use fill_record which permits and assigns only those values whose key match the fields declared by the user in their resources file. I think this mitigates the chances of such attacks, no?

avo/lib/avo/resources/base.rb

Line 448 in 90a64f9

def fill_record(record, params, extra_params: [], fields: nil)

[Paul-Bob]

I think this mitigates the chances of such attacks, no?

Yes, but let's apply the permit directly inside the additional_params method to increase readability.

def additional_params
  params[:fields].permit(@attach_fields.map(&:id))
end

[binarygit]

okayy. Got it 😄

[binarygit]

I pushed a commit that turns the texts to:

Could you take a look @adrianthedev 😄

[Paul-Bob]

@binarygit we're attaching a patron. The field should be named Patron (singular) and not Patrons (plural). Let's verify how we compute the title for the "Attach patron" text and apply the same strategy

[Paul-Bob]

• Some tweaks to improve readability
• Permit params through fields' id instead of fields' name.

[Paul-Bob]

Suggested change

	field_names = @attach_fields.map { |field| field.name.tr(" ", "_").downcase }
	@resource.fill_record(
	@reflection.through_reflection.klass.new,
	additional_params.permit(field_names).merge(
	@resource.fill_record(
	@reflection.through_reflection.klass.new,
	additional_params.merge(

[Paul-Bob]

Suggested change

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

[Paul-Bob]

Suggested change

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

Use additional_params.any? instead of dedicated method

[Paul-Bob]

🎆

[adrianthedev]

One more thing.
Let's see if this permit statement emits a red warning in the console for other params which are sent by the modal.

I'd like to prevent that behavior. Some users when they have a problem with that actions, they will look at that and think that that is the cause of the problem.
So they are led astray.

[Paul-Bob]

I think that behavior depends on the parent's app configuration and can be configured by action_on_unpermitted_parameters (check whole docs here)

Would be great to have a flag to turn it off like: params.permit(on_unnpermited_params: false, ...)