-
-
Notifications
You must be signed in to change notification settings - Fork 264
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: add automatic field detection in resources #3516
base: main
Are you sure you want to change the base?
feature: add automatic field detection in resources #3516
Conversation
Code Climate has analyzed commit 0892908 and detected 0 issues on this pull request. View more on Code Climate. |
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.
This is amazing, @ObiWanKeoni!
I've been testing some scenarios locally, and it's awesome!
I took a look at the code, and it's fantastic that you managed to keep it within the HasFieldDiscovery
concern. This will undoubtedly make our review much easier!
Congratulations on this incredible work!
We'll provide a detailed code review once we validate the DSL and its behavior.
This PR has been marked as stale because there was no activity for the past 15 days. |
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.
Hi @ObiWanKeoni,
Fantastic work here!
I've added a code review with some questions and suggestions, let's keep the discussion going on those.
In addition, I noticed a few issues related to the discovery of tags and trix fields. They don't seem to be working, additionally, those should ideally be discovered by the discover_columns
method rather than discover_associations
WDYT?
I also observed that the has_one_attached :cv
on the user model is generating a files
field instead of a singular file
.
Also noticed some errors on some resources when changing the def fields
method into this:
def fields
discover_columns
discover_associations
end
For example on Avo::Resources::City
Overall, looks great and is working as expected. Let's concentrate on addressing the issues that came up during this review. Thank you!
PS: Let's also clean up the linting warnings, they make the review harder
Co-authored-by: Paul Bob <69730720+Paul-Bob@users.noreply.github.com>
Hi @Paul-Bob!
Thanks for the detailed review! Sorry for the delay in responding - I've been tied up with work stuff.
Completely agree. I've moved the tag and trix field discovery logic into the
Thanks for catching this! I'll take a look at the singular/plural file field issue tomorrow and get that sorted out.
Fixed the field discovery errors in the City resource and a few other affected resources. Please let me know if you notice any others!
Done! I've cleaned up all the linting warnings to make future reviews cleaner and easier to follow. Let me know if you'd like me to clarify anything about the changes I've implemented! |
Appreciate your quick attention to this PR! There remain a few issues that need to be resolved. File / Files
PS: Just noticed this comment:
Trix
Can be tested by replacing def fields
discover_columns
end Tags
Let's also add a test after each fix. Let me know if you need further details! Thank you! |
Hi @Paul-Bob! Really appreciate the detailed commentary here! Here is how I see the remaining changes:
--
I actually just refactored the tests a bit to follow the guidelines in Please also let me know if you notice any gaps in the testing, I'm happy to adjust some more. |
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.
Thanks for the speedy updates, @ObiWanKeoni!
Looks like everything from the initial list of issues I flagged is taken care of.
I actually just refactored the tests a bit to follow the guidelines in CONTRIBUTING.md more closely. Please let me know if you'd like me to add a resource that specifically retains these helpers.
If by that, you mean adding back something like the resource removed here, then I'm all for it! Having a resource we can tinker with would be awesome. Loving the use of with_temporary_items
, but I think we should keep the resource too.
One thing I did notice, on the Avo::Resources::Post
resource, when using:
def fields
discover_columns
end
It looks like tags can't be edited. Not sure what's causing that.
Other than that, we're super close to wrapping up this PR! 🚀
Thank you for the speedy review, @Paul-Bob !
Added back!
🤔 I am not sure to be honest. It seems to vary based on device maybe? because I certainly can edit tags Screen.Recording.2025-01-14.at.10.52.21.AM.movThat said, I noticed that the status seems to not be saving on the Post because the status of '0' is invalid. I'm not sure yet if that should be expected here. It's strange that it's a recognized field and, yet, doesn't quite perform as expected. I'll look into it deeper though |
I've not been very present on this PR but I want to acknowledge the hard work that you've put in this @ObiWanKeoni. |
Tags are on point, I misinterpreted the
I noticed that the options for the select are rendered different: When using The option follow this format: And when using The option follow this format: |
Description
Fixes #3390
This is my working prototype for the issue linked above. It inspects the model much like the resource generator in:
https://github.com/avo-hq/avo/blob/617ead62d09f3f8c04a7438c5fc7cc461f5fcee8/lib/generators/avo/resource_generator.rb
As it gathers all of the columns and associations, it calls the
#field
method to add a field item dynamically. I've added a new User resource with many different use cases of thediscover_columns
anddiscover_associations
methods.Important
As I am still very new to contributing here, I am unfamiliar with some of the design patterns and decisions made thus far in Avo. I would advise that reviewers pay special attention to whether I have broken patterns necessary for cleanliness and readability. There may also be opportunities for refactoring and applying these discovered fields to, say, tab groups but I wanted to get initial feedback before proceeding with the patterns I've established so far.
Checklist:
Screenshots & recording
Manual review steps
Manual reviewer: please leave a comment with output from the test if that's the case.