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

Add the ability to translate submissions (redux) #4219

Merged
merged 17 commits into from
Nov 20, 2024

Conversation

wes-otf
Copy link
Contributor

@wes-otf wes-otf commented Nov 14, 2024

This builds on the #4134 PR that initially introduced machine translations into Hypha. This isolates the translation behavior; putting pip dependencies in a separate requirements-translate.txt and will not attempt any translate imports unless the setting for it is true.

Other small changes are also a full docs page explaining how to install language packages & changing the setting once again from SUBMISSION_TRANSLATIONS_ENABLED to APPLICATION_TRANSLATIONS_ENABLED to reflect the system wide shift away from submission terminology.

Test Steps

  • Ensure Hypha functions completely the same without translations enabled and without argostranslate installed
  • Ensure that when enabled & packages/languages are installed, translations work as expected.

@wes-otf wes-otf added the Type: Minor Minor change, used in release drafter label Nov 14, 2024
@wes-otf wes-otf requested a review from frjo November 14, 2024 20:51
@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 14, 2024

To use hypha the argostranslate package does not need to be installed but right now for tests to pass it needs it (despite all it's functionality being mocked). We could add the install to the CI or I could try to get a functional mock/conditional set up, whatever you think @frjo

@frjo
Copy link
Contributor

frjo commented Nov 14, 2024

Maybe we can use @skipif/@skipunless so the tests only run when APPLICATION_TRANSLATIONS_ENABLED is true?

@@ -24,6 +25,9 @@
from .widgets import MetaTermSelect2Widget, Select2MultiCheckboxesWidget
from .workflow import get_action_mapping

if settings.APPLICATION_TRANSLATIONS_ENABLED:
pass

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this left over test code?

display: inline-block;
height: 1rem;
}
</style>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use tailwind classes for this instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the htmx-request class gets added while the request is processing so I that bit of CSS will need to stay but I moved everything else to tailwind

@frjo
Copy link
Contributor

frjo commented Nov 14, 2024

Overall it looks like a smart way to implement this. We have the feature easily available without putting a burdon on all installs. Nice work!

Will try it out locally and see if I find any edge cases.

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 18, 2024

@frjo your skipUnless solution works well! should we still add the install of argostranslate to the CI so that logic can be tested too or leave it as is? Again all of argos is mocked in my tests so that wouldn't slow it down, it'd just add time on the pip install

@theskumar
Copy link
Member

theskumar commented Nov 19, 2024

I think it's good candidate for #4201 or have it implemented in the client/server mode then use it's translator api.

Advantage of the client/server mode, it will allow someone to easily replace it with another API and/or allow it to run without blocking the main application server.

@frjo
Copy link
Contributor

frjo commented Nov 19, 2024

@wes-otf I think we can run the tests for this in the CI. The pip installs are cached so the effect should be minimal.

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 19, 2024

@frjo CI looks good to go!

@wes-otf
Copy link
Contributor Author

wes-otf commented Nov 19, 2024

@theskumar I totally agree! I think with how isolated the logic is now especially once we're ready to do that implementation I could totally extract it into a plugin

@frjo frjo added the Type: Feature This is something new (not an enhancement of an existing thing). label Nov 20, 2024
@frjo frjo merged commit 7a68e69 into main Nov 20, 2024
7 checks passed
@HyphaApp HyphaApp deleted a comment from sentry-io bot Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature This is something new (not an enhancement of an existing thing). Type: Minor Minor change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants