-
-
Notifications
You must be signed in to change notification settings - Fork 232
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: use custom resource for has_many field #1109
feature: use custom resource for has_many field #1109
Conversation
Code Climate has analyzed commit 08ac86b and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Codecov Report
@@ Coverage Diff @@
## main #1109 +/- ##
==========================================
- Coverage 94.20% 94.18% -0.03%
==========================================
Files 525 530 +5
Lines 10465 10560 +95
==========================================
+ Hits 9859 9946 +87
- Misses 606 614 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a few changes:
- changed the
resource
field option touse_resource
. I did that because in theBaseField
class we already have a@resource
instance variable and we might get some clashes - I changed
related_resource
inApplicationController
to take into account theuse_resource
option from the field. That's why you couldn't see the changes from theHasBaseField
Next up:
- we need to make sure the links in the turbo frame take the users to the proper locations.
For example, when you click on theAttach comment
button it takes you to a page where that new resource is used. Same for the rest of the buttons and links. - make sure the page is translatable using the translation tags
- other misc things like search, attach/detach/create
Getting back to you with some things that were "broken"
Let me know if I'm looking in the wrong place or I'm not reproducing the bugs properly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor tweaks here and there.
- docs needed too
lib/avo/fields/base_field.rb
Outdated
@id.to_s.humanize(keep_id_suffix: true) | ||
end | ||
|
||
def name_from_use_resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_resource
is not a BaseField
property. Can we create a name
method in HasBaseField
that checks for use_resource
and uses that or falls back to super
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach is kinda tricky because we always want to give priority to custom name or to localization... We can have the name method overridden in HasBaseField but there we need to check for custom name and translation too so that would be a duplicated method. Check my last approach for name
Co-authored-by: Adrian Marin <adrian@adrianthedev.com>
Co-authored-by: Adrian Marin <adrian@adrianthedev.com>
…s' of https://github.com/Paul-Bob/avo into improvement/allow_to_specify_resource_on_has_many_fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one thing I'd like us not to push with this PR. The name(from_use_resource: nil)
override in BaseField
.
Also, are all items from this comment checked?
Otherwise, we're good to go! Good job!
lib/avo/fields/has_base_field.rb
Outdated
def name | ||
super(from_use_resource: use_resource&.name) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of a different route where we didn't have to introduce from_use_resource
to the BaseField.name
method.
def name | |
super(from_use_resource: use_resource&.name) | |
end | |
def name | |
return use_resource&.name if use_resource.present? | |
super | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach have an issue, if use resource is present the name: "CustomName"
and translation will be unconsidered. I don't think we want it, we want the name to have the flowing priority:
- Custom name, using name option
- Translation keys
- Name from use resource if present, otherwise
- Name from id
I understand that my first approach and second one are kinda tricky but that was the ways i found to maintain this naming priority to be the same without duplicating the code and the guards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the last approach based on what you suggested
✅ HalfCheked, it keeps the "old" path but actually use the new resource so it's fine
✅ I tested it very quick and looks fine, can you test this one too please?
✔️ Checked, attach and create are even in the spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
Thank you!
Amazing job!
Description
By default,
has_many
will use the field resource.For example
field :comments, as: :has_many
will useCommentResource
.If you want to use a custom resource you need to add the
use_resource: YourCustomResource
option.Make sure to have the custom resource prepared to be used by adding
self.model_class = ::YourModel
Fixes #1103
Checklist: