feature: add automatic field detection in resources

[ObiWanKeoni]

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.

[codeclimate]

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

View more on Code Climate.

[Paul-Bob]

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.

[github-actions]

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

[Paul-Bob]

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

[ObiWanKeoni]

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!

[Paul-Bob]

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]

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.

[Paul-Bob]

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]

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]

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]

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

[ObiWanKeoni]

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>

Great catch! Was a simple enough fix for these cases

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!

Thank you @adrianthedev ! It's seriously hard to overstate how effective Avo has been for me. Y'all are doing some really fantastic work here!

[Paul-Bob]

Thank you for your patience @ObiWanKeoni

Everything that we discussed seems to be all right!

Meanwhile we merged some PRs and there is a small conflict to solve.

On of the merged PRs added this mapping concern https://github.com/avo-hq/avo/blob/main/lib/avo/mappings.rb, do you think this PR can be adjusted to use that concern instead of defining separated mapping constants?

[ObiWanKeoni]

Thank you for your patience @ObiWanKeoni

Everything that we discussed seems to be all right!

Meanwhile we merged some PRs and there is a small conflict to solve.

On of the merged PRs added this mapping concern https://github.com/avo-hq/avo/blob/main/lib/avo/mappings.rb, do you think this PR can be adjusted to use that concern instead of defining separated mapping constants?

Hi @Paul-Bob !

Neat! I like that addition - I went ahead and implemented the change but wasn't sure if the columns to ignore should go there since it's not strictly a "mapping". LMK if you think anything else should change!

[Paul-Bob]

I went ahead and implemented the change but wasn't sure if the columns to ignore should go there since it's not strictly a "mapping". LMK if you think anything else should change!

Awesome! Lets keep those separated since are not commonly used yet.

The PR it's looking good!

Lets ensure that tests are passing before merging it.

I noticed that tests are failing specially on rails version 6.1, any clue?

Also we need to make documentation for this, let me know if you need any assistance setting up the documentation repo.

[ObiWanKeoni]

Lets ensure that tests are passing before merging it.

I noticed that tests are failing specially on rails version 6.1, any clue?

Yeah we had some assertions that expected turbo frames to be loaded so I changed them to be a bit more deterministic in ensuring the turbo frame exists.

Also we need to make documentation for this, let me know if you need any assistance setting up the documentation repo.

Done ✅ (ref)

Please let me know if this is sufficient or if we should get a screen recording or something.

[Paul-Bob]

Amazing work here @ObiWanKeoni thank you!