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

Unique fields in django model translation with Simple History #1299

Closed
natanrmaia opened this issue Jan 24, 2024 · 5 comments
Closed

Unique fields in django model translation with Simple History #1299

natanrmaia opened this issue Jan 24, 2024 · 5 comments

Comments

@natanrmaia
Copy link

Describe the bug
When creating models within django, there are fields that I need to be unique. These same fields can be translated and will also be unique.

However, when creating a migration, the translated fields in the historical table inherit the "Unique" characteristic.
I believe the bug is in Simple History because if I don't create the historical table, django works correctly.
But if I keep it active, I can create new records but I can't update them, it causes a sql error.

models.py:

class OrganizationType(models.Model):
    name        = models.CharField(max_length=100, verbose_name=_('Organization Type Name'), help_text=_('Organization Type Name'), unique=True)
    description = models.TextField(blank=True, null=True, verbose_name=_('Organization Type Description'), help_text=_('Organization Type Description'))

    def __str__(self):
        return self.name

    class Meta:
        verbose_name = _('Organization Type')
        verbose_name_plural = _('Organization Types')
        ordering = ['name']
        db_table = 'organization_type'
        db_table_comment = _('Table of Organization Types')

translations.py

@register_translation(OrganizationType)
class OrganizationTypeTranslationOptions(TranslationOptions):
    fields = ('name', 'description',)
    default_values = {'name': None}
register_history(OrganizationType, table_name='organization_type_history', cascade_delete_history=True, inherit=True)

Excerpt from the migrations file

        migrations.CreateModel(
            name='HistoricalOrganizationType',
            fields=[
                ('id', models.IntegerField(blank=True, db_index=True, help_text='Auto-incremental ID of the record. Do not edit.', verbose_name='ID')),
                ('enabled', models.BooleanField(default=True, help_text='Enable or disable this record. If disabled, it will not be used in the system.', verbose_name='Enabled')),
                ('name', models.CharField(db_index=True, help_text='Organization Type Name', max_length=100, verbose_name='Organization Type Name')),
                ('name_pt_br', models.CharField(help_text='Organization Type Name', max_length=100, null=True, unique=True, verbose_name='Organization Type Name')),
                ('name_en', models.CharField(help_text='Organization Type Name', max_length=100, null=True, unique=True, verbose_name='Organization Type Name')),
                ('description', models.TextField(blank=True, help_text='Organization Type Description', null=True, verbose_name='Organization Type Description')),
                ('description_pt_br', models.TextField(blank=True, help_text='Organization Type Description', null=True, verbose_name='Organization Type Description')),
                ('description_en', models.TextField(blank=True, help_text='Organization Type Description', null=True, verbose_name='Organization Type Description')),
                ('history_id', models.AutoField(primary_key=True, serialize=False)),
                ('history_date', models.DateTimeField(db_index=True)),
                ('history_change_reason', models.CharField(max_length=100, null=True)),
                ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)),
                ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)),
            ],
            options={
                'verbose_name': 'historical Organization Type',
                'verbose_name_plural': 'historical Organization Types',
                'db_table': 'organization_type_history',
                'ordering': ('-history_date', '-history_id'),
                'get_latest_by': ('history_date', 'history_id'),
            },
            bases=(simple_history.models.HistoricalChanges, models.Model),
        ),

To Reproduce

  1. Create a model with a unique field in models.py
  2. Add your translation in translations.py
  3. Register the Simple History historical table
  4. Create any record in the model
  5. Try updating this record in any language
    image
    Screenshot_1

Expected behavior
The migration was created without the 'unique' in the translatable fields without the need for manual editing.

Screenshots
If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

  • OS: Windows 11 Pro
  • Django Simple History Version: 3.4.0
  • Django Version: 5.0.1
  • Database Version: MariaDB 10.4
@natanrmaia natanrmaia changed the title Unique fields in django model translation with Simple Story Unique fields in django model translation with Simple History Jan 24, 2024
@ddabble
Copy link
Member

ddabble commented Feb 17, 2024

This seems like an issue with a library you're using, or some code in your project, as django-simple-history doesn't keep the unique field attribute when creating history models (see models.py) - which you can also see from the fact that the name field in the migration excerpt you included doesn't have the unique attribute.

Furthermore, the fields enabled, name_pt_br, name_en, description_pt_br and description_en in your migration excerpt seem to have been mysteriously inserted by some other code, since none of them appear in the original model, as far as I can see 🤔 So it seems like the problem lies with this "other code" that inserts those fields into the model/migration, and that doesn't remove the unique attributes from name_pt_br and name_en.

Does that seem plausible to you?

@natanrmaia
Copy link
Author

Hey!
I'm using django-model-translation, it is responsible for adding multi-language functionality to django. Even in the django-simple-history documentation itself there is guidance on how to use them together.
Additional columns are generated as I add support for a language to my project.

In this context, it does make sense to maintain the 'unique' clause in the fields. If not, it may happen that my end user translates several different records with the same translation.
The only case that can be ignored is enabled, it is a remnant of code that I forgot to remove.

About the fields you mentioned in my pull. They are classes inherited from django, CharField and TextField.

@ddabble
Copy link
Member

ddabble commented Feb 17, 2024

So TranslationCharField and TranslationTextField are defined by django-model-translation? GitHub doesn't return any search results on them, and they don't seem to be documented either, so I hope you understand I'm a little hesitant to consider adding code for something that almost seems nonexistent 🙂 Code like what you suggested in your PR should be well documented - with comments explaining its purpose and linking relevant documentation, and tests displaying the code's intended behavior. Also, I don't have any evidence that your code actually fixes the issue you've described, but having tests would help with that :)

I'm also generally hesitant to add code for providing compatibility with just any library, as it has a tendency to make the code overall less maintainable, as maintainers have to spend time getting an idea of what each library does before being able to fully understand and then potentially fix/modify the code. Instead, I'm a much bigger fan of updating the documentation with suggestions users can implement themselves - like #579 did. Because if your code does indeed fix your issue, it seems to me that you can simply monkey-patch transform_field() with the code checking for TranslationCharField and TranslationTextField, and then add that code as a suggestion in the documentation, if you want - provided we have evidence that it works, and that the suggestion contains an explanation 🙂

@r3l4x0
Copy link

r3l4x0 commented Feb 26, 2024

Migrations of translation fields from django-modeltranslation are created based on the field attribute translated_field.
django-simple-history copies model fields and sets field._unique = False. As a fix you could check if translated_field is present on a model field and transform it as well. Another alternative would be following code snippet after translator.register and simple_history.register:

import copy
from django.apps import AppConfig, apps as django_apps
from modeltranslation.fields import TranslationField
from simple_history.models import HistoricalChanges, transform_field


class YourAppConfig(AppConfig):
    name = "yourapp"

    def ready(self):
        
        # ...
        # Register models for translation and history here
        # ...

        for model in django_apps.get_models():
            if issubclass(model, HistoricalChanges):
                for field in model._meta.fields:
                    if isinstance(field, TranslationField):
                        translated_field = copy.copy(field.translated_field)
                        transform_field(translated_field)
                        field.translated_field = translated_field

There is no guarantee that nothing else will break with this solution :D.

@ddabble
Copy link
Member

ddabble commented May 6, 2024

Closing stale issue. Please reopen if you'd like to discuss it further 🙂

@ddabble ddabble closed this as not planned Won't fix, can't repro, duplicate, stale May 6, 2024
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

No branches or pull requests

3 participants