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: replace deprecated translation methods #6567

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Oct 28, 2024

📝 Summary

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

✔️ TODO

  • Test this with an actual translation ai setup

mejo-
mejo- previously requested changes Nov 4, 2024
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

I didn't dive too deep into the changes, but a quick testing didn't work as expected. While the fake providers provided by the testing app worked, the providers provided by the translation app were not listed.

What I did to reproduce:

  1. Disable app testing
  2. Enable apps assistant and translate
  3. Run occ translate:download-models

When I load the translation modal from the assistant menu with this PR applied, the dropdowns "translate from" and "translate to" are empty and don't provide any language. Maybe I missed something?

@luka-nextcloud
Copy link
Contributor Author

@mejo- As I understand, the assistant app cannot work without testing app (or AI provider apps). So, disabling testing app means disabling assistant and translation. So, I think the test case doesn't make sense to me.

@juliusknorr
Copy link
Member

There is also translate2, not sure if just an addition or successor. translate doesn't implement the new API.

@marcelklehr From an integration team perspective, is it work to keep both APIs available or is just going with the task processing API and ignoring the old translation API fine.

@juliusknorr juliusknorr added the bug Something isn't working label Nov 21, 2024
@luka-nextcloud
Copy link
Contributor Author

In my opinion, we should go with the task processing API and ignore the old translation API since it might be complicated to maintain when keeping both APIs.

@marcelklehr
Copy link
Member

marcelklehr commented Nov 21, 2024

We opted to implement neither forward nor backward compatibility for the old Translation API in TaskProcessing. The translate2 app only uses the TaskProcessing API, while the translate app uses the old Translation API. I'd recommend to transition to TaskProcessing directly if you don't need to support nc < 30, as the old Translation API is already deprecated and you will need to transition at some point. If it's too much work for you to add support for task processing, we can talk about adding forward and backward compatibility in server, but I'd rather avoid that.

@juliusknorr juliusknorr requested a review from mejo- November 25, 2024 14:52
@luka-nextcloud luka-nextcloud self-assigned this Nov 25, 2024
@mejo-
Copy link
Member

mejo- commented Nov 27, 2024

@luka-nextcloud I'm still unable to successfully test this using real translation providers. Did you test this with real providers, or just with the testing app?

What I did:

  1. Disable app testing
    1. Setup app_api environment as documented in https://docs.nextcloud.com/server/latest/admin_manual/ai/app_api_and_external_apps.html
  2. Setup translate2 app as described in https://docs.nextcloud.com/server/latest/admin_manual/ai/app_translate2.html
  3. Build this branch, select something in text and open the translate assistant modal.

Problems:

  • I cannot select a source language except "detect automatically"
  • Selecting "translate" does nothing.

When opening the assistant modal directly in the header bar (not via Text), translation works as expected, so I guess there's something wrong with the implementation in Text.

This is how the dropdowns for selecting languages in the translation modal via Text look in my tests:

grafik
grafik

@luka-nextcloud luka-nextcloud force-pushed the replace-deprecated-translation-methods branch from e879703 to 08e25e1 Compare December 4, 2024 11:46
@luka-nextcloud
Copy link
Contributor Author

@mejo- I have tested with both apps testing and translate2. BTW, if we use translate2, the from-language selection will only have 1 option "Detect Language"

@marcelklehr
Copy link
Member

Note: integration_openai also implements a translation provider

@max-nextcloud
Copy link
Collaborator

/backport! to stable31

@marcelklehr
Copy link
Member

Installed successulfy now. Translation menu entry in the AI menu doesn't do anything on click:

Error: Could not find initial state translation_can_detect of text
    e http://localhost:8080/apps/text/js/index-CJ7wDv5l.chunk.mjs:1
    data http://localhost:8080/apps/text/js/Editor-DCXGGTKZ.chunk.mjs:2
    vs http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    ds http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    ls http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    _init http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    s http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    Za http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    init http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    d http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    E http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    fc http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    _update http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    r http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    t http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    Va http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    $mount http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    init http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    c http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    i http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    Jg http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    _update http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    u http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    get http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    run http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    Th http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    Zn http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    _s http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
[vue.runtime.esm-DuKaSI2H.chunk.mjs:1:25024](http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs)
TypeError: this.languages is undefined
    fromLanguages http://localhost:8080/apps/text/js/Editor-DCXGGTKZ.chunk.mjs:2
    get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    evaluate http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    lr http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    kA http://localhost:8080/apps/text/js/Editor-DCXGGTKZ.chunk.mjs:2
    _render http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    r http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    t http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    Va http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    $mount http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    init http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    d http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    E http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    fc http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    _update http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    r http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    t http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    Va http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    $mount http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
    init http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    c http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    i http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    Jg http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    _update http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    u http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    get http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    run http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    Th http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    Zn http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
    _s http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2

@juliusknorr
Copy link
Member

I can share credentials for easier testing with the integration_openai provider, just ping me if needed

@max-nextcloud
Copy link
Collaborator

@marcelklehr

Installed successulfy now. Translation menu entry in the AI menu doesn't do anything on click:

That looks like you did not compile it yet.
I told you you would not need to in chat - but I did not understand you were working on a feature branch / pull request. In text we have a compilation task that runs on the main and stable?? branches and provides the compiled js.

Could you try again? We can also jump on a call next week to try it together.

@marcelklehr
Copy link
Member

Yeah, let's do that :)

@marcelklehr
Copy link
Member

notes from test with new build:

On opening:

TypeError: this.toLanguage is null
    fromLanguage Translate.vue:157

when selecting a new "from language":

TypeError: newVal is null
    fromLanguage Translate.vue:157
  • Cannot retranslate after selecting a new "from language"

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Tested this with @marcelklehr and the integration worked fine. There were two errors in the logs:

  • When changing the fromLanguage while toLanguage was not set yet
  • When clearing the toLanguage'.

Both pointed to line 157 of Tranlate.vue. I'll propose a change that will fix them.

In addition it would be good to also clear the result when the fromLanguage is changed - to allow for a recomputation.

Other than that this looks fine. I'll approve so you can fix the remaining things and then merge @luka-nextcloud

@max-nextcloud max-nextcloud removed their assignment Feb 27, 2025
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@luka-nextcloud luka-nextcloud force-pushed the replace-deprecated-translation-methods branch from 08e25e1 to c87a4b8 Compare March 10, 2025 10:33
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 40 lines in your changes missing coverage. Please review.

Project coverage is 37.51%. Comparing base (7d9b968) to head (c87a4b8).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/components/Modal/Translate.vue 0.00% 38 Missing ⚠️
src/components/Assistant.vue 0.00% 1 Missing ⚠️
src/components/Menu/MenuBar.vue 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6567      +/-   ##
==========================================
+ Coverage   37.49%   37.51%   +0.02%     
==========================================
  Files         943      933      -10     
  Lines       42501    42476      -25     
  Branches     1466     1456      -10     
==========================================
  Hits        15935    15935              
+ Misses      25788    25773      -15     
+ Partials      778      768      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@luka-nextcloud
Copy link
Contributor Author

@mejo- Could you please give an approval if everything is okay?

@juliusknorr
Copy link
Member

Lets get this in as @mejo- is unavailable

@juliusknorr
Copy link
Member

juliusknorr commented Mar 10, 2025

@luka-nextcloud Can you double check that this is somehow covered with tests? We should add some if we don't have yet

@luka-nextcloud luka-nextcloud dismissed mejo-’s stale review March 10, 2025 13:34

Max tested it and it worked for Marcel as well.

@juliusknorr juliusknorr merged commit bb4ce6c into main Mar 11, 2025
66 of 68 checks passed
@juliusknorr juliusknorr deleted the replace-deprecated-translation-methods branch March 11, 2025 19:07
@juliusknorr
Copy link
Member

/backport to stable31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

Replace deprecated translation methods
5 participants