Skip to content

Fix: Country field copies wrong clipboard value when copyable: true #4005

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

Merged
merged 6 commits into from
Aug 4, 2025

Conversation

zhephyn
Copy link
Contributor

@zhephyn zhephyn commented Aug 2, 2025

Fixes #3938

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

When we have to write the same logic in multiple places, it's usually a sign that we're applying it in the wrong layer. This approach can lead to inconsistent behavior in the future and subtle bugs that require debugging sessions to untangle.

Could we make field.value return the correct value for the country field? Maybe we also need to introduce a new method, something like code_value to return the raw DB value, and keep value for the display-friendly version. Please investigate the issue and let me know if there are any complications or edge cases.

Thanks for digging into this!

@zhephyn
Copy link
Contributor Author

zhephyn commented Aug 4, 2025

I had done some extensive digging around on this. Gonna provide a more detailed version for all the potential solutions I had considered. Thanks again for the review

@zhephyn
Copy link
Contributor Author

zhephyn commented Aug 4, 2025

Some more context.
I've refactored the approach such that we override the value method in the country_field.rb file.

      def value
        stored_value = super

        if @display_code
          stored_value
        else
          countries[stored_value]
        end
      end

As for the cause of the bug, when i dug around to see why the other fields which utilized the copyable option were working as expected yet this wasn't the case for the country field, i discovered that a similarity between all these fields was that when copyable is set to true, the value we copy is the one saved in the database. For the rest of the fields such as the text field, there was no problem with this.

For the country field however, since we save the country code in the database and not the country name, as per the current implementation of the copyable option, we would always copy the country code and not the country name since this is what was saved in the database.

By this logic, another approach to solving the bug would be to change the value stored in the database for the country field to be the country name instead of the country code by changing the select_options method from this:

      def select_options
        if @display_code
          countries.map do |code, name|
            [code, code]
          end
        else
          countries.map do |code, name|
            [name, code]
          end
        end
      end

to this:

      def select_options
        if @display_code
          countries.map do |code, name|
            [code, name]
          end
        else
          countries.map do |code, name|
            [name, name]
          end
        end
      end

That allows us to copy the country name without making any other changes in the code base after all, its what is saved in the database.

Though the issue with this approach is that while country codes are standardized regardless of the language, that is not the case for country names.

@zhephyn
Copy link
Contributor Author

zhephyn commented Aug 4, 2025

new changes for this are up!

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, @zhephyn! Great explanation and refactor! I probably would have taken the same strategy and approach

@Paul-Bob Paul-Bob added the Fix label Aug 4, 2025
@Paul-Bob Paul-Bob merged commit 5bd4ddc into avo-hq:main Aug 4, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix country fields wrong clipboard value when copyable: true
2 participants