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: add automatic field detection in resources #3516

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ObiWanKeoni
Copy link
Contributor

@ObiWanKeoni ObiWanKeoni commented Dec 11, 2024

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 the discover_columns and discover_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:

  • 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

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.

Copy link

codeclimate bot commented Dec 11, 2024

Code Climate has analyzed commit 0892908 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Contributor

@Paul-Bob Paul-Bob left a 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.

@adrianthedev adrianthedev mentioned this pull request Dec 18, 2024
4 tasks
Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

Copy link
Contributor

@Paul-Bob Paul-Bob left a 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

lib/avo/concerns/has_field_discovery.rb Outdated Show resolved Hide resolved
lib/avo/concerns/has_field_discovery.rb Outdated Show resolved Hide resolved
lib/avo/concerns/has_field_discovery.rb Outdated Show resolved Hide resolved
lib/avo/concerns/has_field_discovery.rb Outdated Show resolved Hide resolved
@ObiWanKeoni
Copy link
Contributor Author

Hi @Paul-Bob!

Fantastic work here!
I've added a code review with some questions and suggestions, let's keep the discussion going on those.

Thanks for the detailed review! Sorry for the delay in responding - I've been tied up with work stuff.

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?

Completely agree. I've moved the tag and trix field discovery logic into the discover_columns method where it makes more sense architecturally.

I also observed that the has_one_attached :cv on the user model is generating a files field instead of a singular file.

Thanks for catching this! I'll take a look at the singular/plural file field issue tomorrow and get that sorted out.

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

Fixed the field discovery errors in the City resource and a few other affected resources. Please let me know if you notice any others!

PS: Let's also clean up the linting warnings, they make the review harder

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!

@ObiWanKeoni ObiWanKeoni requested a review from Paul-Bob January 10, 2025 02:31
@Paul-Bob
Copy link
Contributor

Paul-Bob commented Jan 10, 2025

Thanks for the detailed review! Sorry for the delay in responding - I've been tied up with work stuff.

Appreciate your quick attention to this PR!


There remain a few issues that need to be resolved.

File / Files

  • has_one_attached :cv is assigning the fields as: :files (multiple) instead of as: :file (single). Ensure it assigns singular attachment as intended. (tested on a Avo::Resources::FieldDiscoveryUser edit page, the upload should be "Choose File" instead "Choose Files"

  • has_one_attached :cv is generating the field id cv_attachment instead of :cv. Adjust to follow the file / fields name conventions expectation. (tested on Avo::Resources::FieldDiscoveryUser)

PS: Just noticed this comment:

Thanks for catching this! I'll take a look at the singular/plural file field issue tomorrow and get that sorted out.

Trix

  • Trix is rendered twice once extended and once with the more content button, only last should be rendered.

Can be tested by replacing def fields inside Avo::Resources::Event

  def fields
    discover_columns
  end

Tags

  • While descovering tags there is an error (undefined local variable or method 'field_name' for an instance of Post) around this line field tag_field_name(association_name), as: :tags, **@field_options.merge(acts_as_taggable_on: field_name)

Let's also add a test after each fix.

Let me know if you need further details! Thank you!

@ObiWanKeoni
Copy link
Contributor Author

Hi @Paul-Bob!

Really appreciate the detailed commentary here! Here is how I see the remaining changes:

File / Files

  • has_one_attached :cv is assigning the fields as: :files (multiple) instead of as: :file (single). Ensure it assigns singular attachment as intended. (tested on a Avo::Resources::FieldDiscoveryUser edit page, the upload should be "Choose File" instead "Choose Files"
  • has_one_attached :cv is generating the field id cv_attachment instead of :cv. Adjust to follow the file / fields name conventions expectation. (tested on Avo::Resources::FieldDiscoveryUser)

Trix

  • Trix is rendered twice once extended and once with the more content button, only last should be rendered.

Tags

  • While descovering tags there is an error (undefined local variable or method 'field_name' for an instance of Post) around this line field tag_field_name(association_name), as: :tags, **@field_options.merge(acts_as_taggable_on: field_name)

--

Let's also add a test after each fix.

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.

Please also let me know if you notice any gaps in the testing, I'm happy to adjust some more.

Copy link
Contributor

@Paul-Bob Paul-Bob left a 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! 🚀

@ObiWanKeoni
Copy link
Contributor Author

Thanks for the speedy updates, @ObiWanKeoni!

Thank you for the speedy review, @Paul-Bob !

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.

Added back!

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.

🤔 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.mov

That 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

@adrianthedev
Copy link
Collaborator

I've not been very present on this PR but I want to acknowledge the hard work that you've put in this @ObiWanKeoni.
Thank you!

@Paul-Bob
Copy link
Contributor

🤔 I am not sure to be honest. It seems to vary based on device maybe? because I certainly can edit tags

Tags are on point, I misinterpreted the '0' is not a valid status error and thought that is tags related.

That 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 noticed that the options for the select are rendered different:

When using field :status, as: :select, enum: ::Post.statuses:

The option follow this format: <option value="draft">draft</option>

And when using discover_columns:

The option follow this format: <option value="0">draft</option>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add automatic field detection in resources
3 participants