Skip to content

Add pretty_generated option for code fields #3782

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

zhephyn
Copy link
Contributor

@zhephyn zhephyn commented Apr 8, 2025

Description

This PR adds a pretty generated option for code fields such that;
Instead of:
field :body, as: :code, format_using: -> { JSON.pretty_generate(JSON.parse(value.to_json)) }, update_using: -> { JSON.parse(value) }

We can use this syntax instead:
field :body, as: :code, pretty_generated: true

Fixes #3727

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

Copy link

codeclimate bot commented Apr 8, 2025

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

View more on Code Climate.

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 9, 2025

@Paul-Bob. I ran into the Rubocop errors below:

lib/avo/fields/code_field.rb:17:11: W: [Correctable] Lint/UselessAssignment: Useless assignment to variable - format_using.
  format_using = -> {
  ^^^^^^^^^^^^
  [Correctable] Lint/UselessAssignment: Useless assignment to variable - update_using.
  
  Raw Output:
lib/avo/fields/code_field.rb:21:11: W: [Correctable] Lint/UselessAssignment: Useless assignment to variable - update_using.
  update_using = -> {
  ^^^^^^^^^^^^

Which I solved by changing from this:

if @pretty_generated
     format_using = -> {JSON.pretty_generate(JSON.parse(value.to_json))}
     update_using = -> { JSON.parse(value)}
end

To this in the code_field.rb file:

if args[:pretty_generated]
   args[:format_using] ||= lambda do
      JSON.pretty_generate(JSON.parse(value.to_json))
   end

    args[:update_using] ||= lambda do
       JSON.parse(value)
    end
 end

This code solved the error but it uses a lambda. I don't know if it could use improvement or not.

Currently, the prettified version of the JSON is displayed both on the edit and show page. To test this out in my browser, i added an seo field to the post resource.

Show page
image

Edit page
image

The edit function works as expected and the Pretty rendering of the JSON doesn't break on updating.

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 9, 2025

Secondly, I was proposing writing tests for this functionality. 2 tests in particular:

  1. One tests that the prettified version of the JSON is displayed on the show page after filling the field.

  2. The other tests that, the JSON field displayed on the Edit page is prettified, its value can be updated(even here the field should be prettified) and that finally the JSON displayed thereafter is prettified.

For the first test, I have some progress, though, I'm having issues regarding how to fill the code field with JSON.
On simulating the current test in the browser, it shows that the code field indeed is filled with JSON, however the JSON is not pretiffied. I could use your help here
image

it "correctly formats JSON code on create and displays it in a pretty way on the show page" do
      visit "/admin/resources/cities/new"
      wait_for_loaded

      within find_field_element("metadata") do
        fill_in_editor_field(JSON.pretty_generate(metadata.to_json))
      end
      save
      expect(find_field_value_element("metadata")).to have_text(JSON.pretty_generate(metadata))
    end

I think the issue here is with these two lines:

fill_in_editor_field(JSON.pretty_generate(metadata.to_json))
expect(find_field_value_element("metadata")).to have_text(JSON.pretty_generate(metadata))

Full test code

  describe "with pretty_generated option for a JSON code field" do
    before do 
      Avo::Resources::City.with_temporary_items do
        field :metadata, as: :code, pretty_generated: true
      end
    end

    after do 
      Avo::Resources::City.restore_items_from_backup
    end

    let (:metadata) do 
      {
        
        "name": "New York",
        "country": "United States",
        "population": 8419600,
        "coordinates": {
          "latitude": 40.7128,
          "longitude": -74.006
        },
        "timezone": "America/New_York",
        "climate": {
          "type": "humid subtropical",
          "average_temperature_celsius": 13.1
        },
        "points_of_interest": [
          "Statue of Liberty",
          "Central Park",
          "Empire State Building"
        ]
      }
    end
    it "correctly formats JSON code on create and displays it in a pretty way on the show page" do
      visit "/admin/resources/cities/new"
      wait_for_loaded

      within find_field_element("metadata") do
        fill_in_editor_field(JSON.pretty_generate(metadata.to_json))
      end
      save
      expect(find_field_value_element("metadata")).to have_text(JSON.pretty_generate(metadata))
    end

    it "correctly updates the JSON on edit" do
    end
  end

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 9, 2025

@Paul-Bob I'll then proceed with the tests if they are needed and then update the documentation.
PS: Sorry about the rambling in case it got out of hand :)

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 submitting this PR. Let's add tests and documentation, and it'll be good to go!

@Paul-Bob
Copy link
Contributor

Secondly, I was proposing writing tests for this functionality. 2 tests in particular:

Regarding the tests, let's commit those to the PR

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 10, 2025

I had stumbled upon an issue I could not solve in my first draft of one of the tests. It involved how to fill in the JSON in the code field. I attached the pictures and the code for context in my earlier 2 comments above. If you could help me look at it and get unstuck.

@Paul-Bob
Copy link
Contributor

I had stumbled upon an issue I could not solve in my first draft of one of the tests. It involved how to fill in the JSON in the code field. I attached the pictures and the code for context in my earlier 2 comments above. If you could help me look at it and get unstuck.

It's ok to commit code that's still a work in progress. We won't merge until it's ready. But it's tough for me to test things and give feedback if the code isn't committed. Otherwise, I'd need to recreate your setup by copying snippets from the comments, which isn't ideal

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 10, 2025

Okay. Let me commit the code from the tests shortly so that you can look at them.

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 14, 2025

Currently, the test is failing with a parser error

CodeField with pretty_generated option for a JSON code field correctly formats JSON code on create and displays it in a pretty way on the show page
     Failure/Error: args[:update_using] ||= -> {JSON.parse(value)}
     
     JSON::ParserError:
       unexpected token at end of stream '"{\n        \"name\": \"New York'
     # /usr/share/rvm/gems/ruby-3.3.1/gems/json-2.10.2/lib/json/common.rb:248:in `parse'
     # /usr/share/rvm/gems/ruby-3.3.1/gems/json-2.10.2/lib/json/common.rb:248:in `parse'
     # ./lib/avo/fields/code_field.rb:16:in `block in initialize'
     # ./lib/avo/execution_context.rb:69:in `instance_exec'
     # ./lib/avo/execution_context.rb:69:in `handle'

@Paul-Bob
Copy link
Contributor

Hi @zhephyn,

The issue was that when visiting a form where the DB value for the JSON was nil, the format_using was converting it to the string "null". The test was then appending the metadata JSON after this "null" string.

I've updated the format_using to:

args[:format_using] ||= -> { value.blank? ? value : JSON.pretty_generate(value) }

Now, it properly checks if the value is blank and avoids pretty-generating it in such cases.

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 14, 2025

@Paul-Bob. Okay. This really gave me a hard time to figure out. Is there a specific approach you used when debugging that allowed you to know what the problem was? I had made changes to my city.rb file locally to use the pretty_generated option instead of format_using and update_using. So I don't get why the test was still failing.

Secondly, I have a working copy of the documentation changes on my local machine that I'm gonna push momentarily. Apart from this, is there anything else you'd want me to help out with before we merge the PR?

PS: Thanks so much for your patience🙏

@Paul-Bob
Copy link
Contributor

Is there a specific approach you used when debugging that allowed you to know what the problem was? I had made changes to my city.rb file locally to use the pretty_generated option instead of format_using and update_using. So I don't get why the test was still failing.

Yes, I did exactly that, on the City resource, I modified the metadata field to use the pretty_generated: true option instead of format_using and update_using.

When I visited the creation form page, the metadata field was pre-filled with the string "null" rather than being empty as expected.


Secondly, I have a working copy of the documentation changes on my local machine that I'm gonna push momentarily.

Awesome!


Apart from this, is there anything else you'd want me to help out with before we merge the PR?

I think that's pretty much it, everything is working, tested, and documented. I'll give it one final review, push any tweaks if necessary, and aim to merge it later this week.

Thanks so much for this contribution, you're on a hot streak! 😄


PS: Thanks so much for your patience🙏

Don't worry about it, take your time

@zhephyn
Copy link
Contributor Author

zhephyn commented Apr 14, 2025

I've pushed the documentation PR to the documentation repository.

Plus, i want to take on another issue. Is there one you have preference for that you want to be worked on this week? If yes, i could take on that and start on it.

Otherwise, let me browse through them and pick one to work on.

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

Successfully merging this pull request may close these issues.

Add pretty_generated option for code fields
2 participants