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

Change MT provider selection to dropdowns #2642

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

PeterNerlich
Copy link
Contributor

Short description

Offer setting the preferred machine translations provider for each language.

Proposed changes

  • Add the field preferred_mt_provider to the LanguageTreeNode model.
  • All critical places should still access the property mt_provider. It has been altered to return the provider for the name in preferred_mt_provider if available for the language, or the next best entry from the mt_providers list, or None if no providers can translate to that language.
  • In the machine translations dashboard, replace the checkboxes which language to enable machine translations for with a per-language dropdown for the preferred translations provider.
  • Re-arrange the machine translations dashboard so the list of dropdowns has enough room downwards.

Side effects

  • We create the form fields by generating them on the fly per language when required by the template. Because the languages are different and changing between and potentially within regions, they are not part of the formal model declaration.
    This means that data regarding them gets rejected from cleaned_data, and we circumvent that by initially saving the raw data the form gets initialised with as uncleaned_data. This is surely not the best way to handle this, so please feel free to suggest alternative methods – we decided not to waste time on researching and re-discovering long known practices or reinventing the wheel any more than this, and instead rely on you, at this moment. 😛
  • The translation providers now don't exist only as classes referenced in a list in constants anymore, but also with their names as choices in the LanguageTreeNode model (the preferred provider setting). Seeing how we have a rather loose dependency on it (only accessing via the mt_provider property to fall back to the next best available provider in case), we might also lift that restriction and just make it a field for arbitrary strings, so we don't need database migrations when we add, remove or rename providers.
  • Loosely related to the previous point: We might want to introduce slugs or similar identifiers for translation providers to separate this setting from the user facing name and make this more robust.

Resolved issues

Fixes: #2586

@JoeyStk


Pull Request Review Guidelines

@PeterNerlich PeterNerlich requested a review from a team as a code owner February 6, 2024 21:08
Copy link

codeclimate bot commented Feb 6, 2024

Code Climate has analyzed commit 4df9b57 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 62.8% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.1% (0.0% change).

View more on Code Climate.

@MizukiTemma
Copy link
Member

  • We create the form fields by generating them on the fly per language when required by the template. Because the languages are different and changing between and potentially within regions, they are not part of the formal model declaration.
    This means that data regarding them gets rejected from cleaned_data, and we circumvent that by initially saving the raw data the form gets initialised with as uncleaned_data. This is surely not the best way to handle this, so please feel free to suggest alternative methods – we decided not to waste time on researching and re-discovering long known practices or reinventing the wheel any more than this, and instead rely on you, at this moment. 😛

Is it the problem that each region has a defferent set/number of languages? Then Model Form Functions may be an option (if you haven't yet tried). This is what is being used for POICategoryTranslation and we can handle exactly as many forms as the number of non-root-languages of the region by setting max_num and min_num.

Though we can implement this not now but later as follow-up refactoring because the current implementation is working anyway.

Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

I approve this PR already, for the actual implementation is working as expected 😸

Let's see how it will be with #2637 (it looks already good as far as I tried locally merged) 😁

Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Thanks, this looks mostly good to me. I also tested to add a new translation provider and everything seems to work well.

I just feel a bit uneasy about the form changes. I think you could use a django MultiValueField to represent multiple languages with each different choices.

@david-venhoff
Copy link
Member

I tried to use the multiValueField, but I am also not sure if it is completely right for this case 🙈

This is what I hacked together (and quite incomplete)

class LanguagePreferencesWidget(MultiWidget):
    def __init__(self, languages_dict, attrs=None):
        widgets = {}
        for slug, language_options in languages_dict.items():
            widgets[slug] = Select(
                attrs=attrs,
                choices=language_options.choices,
            )
        super().__init__(widgets, attrs)

    def decompress(self, value):
        print(value)
        if value:
            return list(value.values())
        return [None for _ in self.widgets]


class LanguagePreferences(MultiValueField):
    def __init__(self, languages_dict, **kwargs):
        fields = []
        for slug, language_options in languages_dict.items():
            fields.append(forms.ChoiceField(
                label=language_options.language_tree_node.translated_name,
                choices=language_options.choices,
                initial=language_options.initial,
            ))
        self.slugs = list(languages_dict.keys())
        initial_data = {slug: language_options.initial for slug, language_options in languages_dict.items()}

        super().__init__(fields, widget=LanguagePreferencesWidget(languages_dict), error_messages={}, initial=initial_data, **kwargs)

Alternatively, maybe you could maybe remove the get_language_fields method and set the fields in __init__? Then there is no need for uncleaned_data at least

@PeterNerlich
Copy link
Contributor Author

Alternatively, maybe you could maybe remove the get_language_fields method and set the fields in __init__? Then there is no need for uncleaned_data at least

I'm not sure, originally fields are defined as static for the class and their names are listed in the Meta class, which I haven't tried accessing from within the __init__ method. I just assumed this wouldn't be possible. Might be worth a try

@MizukiTemma MizukiTemma added the ‼️ prio: high Needs to be resolved ASAP. label Feb 20, 2024
Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Given that this has prio: high now, I will just approve it, we can always change the form later if we find problems with this approach 🙈

Co-authored-by: Peter Nerlich <peter.nerlich@tuerantuer.org>
@MizukiTemma MizukiTemma force-pushed the feature/add-dropdown-to-mt-selection branch from 0833f5a to 4df9b57 Compare February 22, 2024 18:10
@MizukiTemma
Copy link
Member

@PeterNerlich @JoeyStk
Sorry, I take the joy of pressing the merge button from you 😿 to test Google Translate with this PR together.

@MizukiTemma MizukiTemma merged commit 1d841af into develop Feb 22, 2024
5 checks passed
@MizukiTemma MizukiTemma deleted the feature/add-dropdown-to-mt-selection branch February 22, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
‼️ prio: high Needs to be resolved ASAP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Language Selection of Maschine Translation
4 participants