-
Notifications
You must be signed in to change notification settings - Fork 7
Add finalize dref api #2558
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
base: project/dref-translation
Are you sure you want to change the base?
Add finalize dref api #2558
Conversation
89a6cb8 to
7bebad0
Compare
9384f6a to
6745b93
Compare
8de1330 to
9d7c227
Compare
6745b93 to
55d133b
Compare
main/lock.py
Outdated
|
|
||
| OPERATION_LEARNING_SUMMARY = _BASE + "-operation-learning-summary-{0}" | ||
| OPERATION_LEARNING_SUMMARY_EXPORT = _BASE + "-operation-learning-summary-export-{0}" | ||
| DREF_TRANSLATION = _BASE + "-dref-translation-{0}" |
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.
we also need to add model table name in the key...
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.
Change DREF_TRANSLATION to MODEL_TRANSLATION
"-{db_table_name}-translation-{0}"
dref/utils.py
Outdated
| """ | ||
| Trigger translation task with Redis lock. | ||
| """ | ||
| with redis_lock(key=RedisLockKey.DREF_TRANSLATION, id=instance.pk) as acquired: |
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.
The lock needs to be in the job function, not the trigger function
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.
Any reason on why we are modifying existing migration file?
How we are planning to run this in local and already deployed instances
dref/serializers.py
Outdated
| to = {u.email for u in validated_data["users"]} | ||
| else: | ||
| to = None | ||
| # set original language |
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.
Is this a TODO?
dref/serializers.py
Outdated
| def create(self, validated_data): | ||
| dref = validated_data["dref"] | ||
| language = validated_data.get("original_language") | ||
| if language != dref.original_language and language != dref.translation_module_original_language: |
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.
Do we need a or here?
dref/serializers.py
Outdated
| # else copy from dref | ||
| dref = validated_data["dref"] | ||
| language = validated_data.get("original_language") | ||
| if language != dref.original_language and language != dref.translation_module_original_language: |
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.
Do we need a or here?
dref/views.py
Outdated
| raise serializers.ValidationError(gettext("Cannot be finalized because it is already %s") % dref.get_status_display()) | ||
| if not is_translation_complete(dref): | ||
| trigger_translation(dref) | ||
| raise serializers.ValidationError(gettext("Translation is not completed. Please try again later.")) |
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.
Let's add something like "the translation is triggered, so wait and try again" @udaynwa
dref/views.py
Outdated
| if dref.status in [Dref.Status.FINALIZED, Dref.Status.APPROVED]: | ||
| raise serializers.ValidationError(gettext("Cannot be finalized because it is already %s") % dref.get_status_display()) | ||
| if not is_translation_complete(dref): | ||
| trigger_translation(dref) |
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.
Are we triggering related tables translation in another PR?
| # Check request language | ||
| if current_language not in valid_languages: | ||
| raise serializers.ValidationError( | ||
| gettext(f"Language must be either '{valid_languages[0]}' or '{valid_languages[1]}'.") |
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.
| gettext(f"Language must be either '{valid_languages[0]}' or '{valid_languages[1]}'.") | |
| gettext(f"Cannot create OperationalUpdate with the current language {current_language}, language must be either '{valid_languages[0]}' or '{valid_languages[1]}'.") |
@udaynwa Verify?
| gettext(f"Language must be either '{valid_languages[0]}' or '{valid_languages[1]}'.") | ||
| ) | ||
| # Check payload language | ||
| language = validated_data.get("starting_language") |
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.
| language = validated_data.get("starting_language") | |
| starting_language = validated_data.get("starting_language") |
Firstly, We need to check if the language and body stating langauge are same or not!
| valid_languages = [dref.starting_language, dref.translation_module_original_language] | ||
| # Check request language | ||
| if current_language not in valid_languages: | ||
| raise serializers.ValidationError( |
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.
Update message
| permission_classes=[permissions.IsAuthenticated, DenyGuestUserPermission], | ||
| ) | ||
| def finalize(self, request, pk=None, version=None): | ||
| field_report = self.get_object() |
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.
| field_report = self.get_object() | |
| final_report = self.get_object() |
Addresses
Changes
Checklist
Things that should succeed before merging.
Release
If there is a version update, make sure to tag the repository with the latest version.