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

737 i18n update all missing translations initial PR #763

Merged
merged 6 commits into from
May 31, 2024

Conversation

wandergithub
Copy link
Contributor

🔗 Issue

#737

✍️ Description

These are general changes, now I'm planning to address each resource (Controller-Model-View Packs) individually on separate PRs. (what do you think about this?)

  • adds error translation to en.yml
  • adds the error translation where it applies
  • adds contactsController translation and updates it on test(This code is not currently being used on route)
  • Important: I think we should Set config of i18n to raise errors while testing when missing a translation after I address all errors it raises. (what do you think about this?)
    At:config/enviroments/test.rb
    # Raises error for missing translations.

config.i18n.raise_on_missing_translations = true

📷 Screenshots/Demos

- adds error translation to en.yml
- adds the error translation where applies
- adds contactsController translation and updated on test(This code is not currently being used on route)
- Important: Sets config of i18n to raise errors when missing a translation
@wandergithub wandergithub changed the title 737 i18n update controllers 737 i18n update all missing translations initial PR May 29, 2024
@kasugaijin
Copy link
Collaborator

@wandergithub I agree with the approach to break it up like that. I also think it would be useful to add the missing translation exception in test and dev environments so we get quick feedback. We'll want to handle it more gracefully in production.
FYI two fixes required - test is broken, and run rails standard:fix to lint.

@kasugaijin
Copy link
Collaborator

@wandergithub is it worth adding that configuration in this 'general' PR?

@kasugaijin
Copy link
Collaborator

I see this test is failing in CI

Error:
  UsersTest#test_user_can_log_out:
  Ferrum::ProcessTimeoutError: Browser did not produce websocket url within 20 seconds, try to increase `:process_timeout`. See https://github.com/rubycdp/ferrum#customization
      test/system/users_test.rb:12:in `block in <class:UsersTest>'
  
  bin/rails test test/system/users_test.rb:11

Does it pass or fail locally for you? Run rails test:system

@wandergithub
Copy link
Contributor Author

wandergithub commented May 29, 2024

@wandergithub is it worth adding that configuration in this 'general' PR?

You are referring to I18n (if not clarify please).
config.i18n.raise_on_missing_translations = true
I would add it once I solve a few missing translations. If not then it's going to raise errors. (Make noise)

@wandergithub
Copy link
Contributor Author

I see this test is failing in CI

Error:
  UsersTest#test_user_can_log_out:
  Ferrum::ProcessTimeoutError: Browser did not produce websocket url within 20 seconds, try to increase `:process_timeout`. See https://github.com/rubycdp/ferrum#customization
      test/system/users_test.rb:12:in `block in <class:UsersTest>'
  
  bin/rails test test/system/users_test.rb:11

Does it pass or fail locally for you? Run rails test:system

Systems tests are falling for me locally I don't know why. But I though It wouldn't be a problem since I didn't change anything related. Seems I was wrong. Any Idea of what's happening here:
image

@kasugaijin
Copy link
Collaborator

A brief look and it appears it could be a WSL gremlin. Try https://github.com/orgs/community/discussions/52722

@wandergithub
Copy link
Contributor Author

I just confirmed all test are passing locally. whats next step now? @kasugaijin
image

@wandergithub
Copy link
Contributor Author

I see this test is failing in CI

Error:
  UsersTest#test_user_can_log_out:
  Ferrum::ProcessTimeoutError: Browser did not produce websocket url within 20 seconds, try to increase `:process_timeout`. See https://github.com/rubycdp/ferrum#customization
      test/system/users_test.rb:12:in `block in <class:UsersTest>'
  
  bin/rails test test/system/users_test.rb:11

Does it pass or fail locally for you? Run rails test:system

Systems tests are falling for me locally I don't know why. But I though It wouldn't be a problem since I didn't change anything related. Seems I was wrong. Any Idea of what's happening here: image

regarding this. The link you provided is a different error. I didn't have chromium. Installing it solves the issue. Can we do something like putting it as a requirement on the readme so new devs like me don't experience what I did? Give me the green light and I'll send the PR.

@kasugaijin
Copy link
Collaborator

Ah ok good job. Yes you can make a PR to add to the readme to install chromium if that error is encountered.

for this PR, you could also resolve the existing errors then add the config to raise an error for missing translations. Or if you want to do that separately we can merge this one. Your call. Then continue on with the other PRs as you outlined above.

@maebeale maebeale requested a review from kasugaijin May 31, 2024 20:25
Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kasugaijin kasugaijin merged commit 548b852 into rubyforgood:main May 31, 2024
5 checks passed
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.

2 participants