-
Notifications
You must be signed in to change notification settings - Fork 50
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
DEV: Move saving of translations into base class #203
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nattsw
commented
Feb 6, 2025
Comment on lines
-29
to
-32
if !post.custom_fields_clean? | ||
post.save_custom_fields | ||
post.publish_change_to_clients!(:revised) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moved to base.rb's save_detected_locale
nattsw
commented
Feb 6, 2025
Comment on lines
-98
to
-106
# the translate button appears if a given post is in a foreign language. | ||
# however the title of the topic may be in a different language, and may be in the user's language. | ||
# if this is the case, when this is called for a topic, the detected_lang will be the user's language, | ||
# so the user's language and the detected language will be the same. For example, both could be "en" | ||
# google will choke on this and return an error instead of gracefully handling it by returning the original | ||
# string. | ||
# --- | ||
# here we handle that situation by returning the original string if the source and target lang are the same. | ||
return detected_lang, get_text(topic_or_post) if (detected_lang&.to_s.eql? I18n.locale.to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moved into base.rb so that all subclasses won't face the same issue.
nbianca
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Since a long long time ago, we do the following in the
Job::DetectTranslation
:discourse-translator/plugin.rb
Lines 126 to 130 in 4213639
.detect
has a side effect of assigning the detected language e.g. "en" into a custom field.The above is slightly confusing as there should be just one place that sets and saves the value. Additionally, the knowledge of the usage custom fields are in every subclass of the base translator.
Change
The following changes allow the saving of translation metadata to be in the single base class instead of sprinkled in all the subclasses. This is to prepare for the move from custom fields to proper tables.
This PR does two things:
detect!
(and alsotranslate!
) in translator subclasses (Amazon, Google, Microsoft, etc) which will set the value from the API all the time. The base class invokesdetect!
.discourse-translator/app/services/discourse_translator/google.rb
Lines 77 to 85 in 4c5d8e7
detect
to return the stored value or invoke the!
variant to get the value if it does not exist.discourse-translator/app/services/discourse_translator/base.rb
Lines 51 to 57 in 4c5d8e7
There is already test coverage for this refactor.