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

fix/ADF-1827/sync-translations-on-authoring #490

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

shaveko
Copy link
Contributor

@shaveko shaveko commented Nov 27, 2024

related to https://oat-sa.atlassian.net/browse/ADF-1827

Description

Once originResourceUri is provided to the authoring call, sync the translations

How to test

checkout related tao branches

Steps to reproduce / check if issue is fixed:

  • create an item
  • create test
  • add item to the test
  • create test translation to some language, save it
  • add translation to the item in that language
  • go back to tests and edit the translation of the test you have created

label should exist for the item

Copy link

github-actions bot commented Nov 27, 2024

Front-end summary Node 18

💯 Total ✅ Passed ⏭️ Skipped ❌ Failed
0 0 0 0

@shaveko shaveko requested a review from jsconan November 27, 2024 13:50
@@ -251,6 +252,7 @@ public function authoring()
);
}
if ($this->getRequestParameter('originResourceUri') !== null) {
$this->getTranslationSyncService()->syncById($this->getRequestParameter('originResourceUri'));
Copy link
Contributor

Choose a reason for hiding this comment

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

@shaveko the FE already calls and endpoint translation/sync to call the syncByRequest. I am not sure also asking the BE to do the same is a good idea 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please check comment to the ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

Now yes, if we remove the FE part of the code, it makes sense to do it on the BE side. When I checked the PRs, there was only the BE part introduced, so the call would be redundant and this sync process is quite expensive.

@shaveko shaveko requested review from shpran and gabrielfs7 November 27, 2024 16:12
Copy link
Contributor

@gabrielfs7 gabrielfs7 left a comment

Choose a reason for hiding this comment

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

LGTM, Good job!

Code review only

  • [] New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • [] Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

Code looks good, and it runs fine.

One note, though, when I add an item to the main test, it does not show up in the translated test.

Review Checklist

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code respects code style rules
  • New code respects best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Pull request's target is not master
  • Commits are following conventional commits
  • Commits messages are meaningful
  • Commits are atomic

Copy link

github-actions bot commented Dec 5, 2024

Version

Target Version 16.4.3
Last version 16.4.2

There are 0 BREAKING CHANGE, 0 feature, 2 fixes

@shaveko shaveko merged commit be66546 into develop Dec 5, 2024
4 checks passed
@shaveko shaveko deleted the fix/ADF-1827/sync-translations-on-authoring branch December 5, 2024 15:58
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.

3 participants