Criar researcher.models.AffiliationMixin e article.models.ArticleAffiliation#1339
Criar researcher.models.AffiliationMixin e article.models.ArticleAffiliation#1339robertatakenaka merged 5 commits intomainfrom
Conversation
…rticle.models Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements a dual-mode affiliation system for articles, supporting both structured organization references and unstructured raw data fields. It introduces AffiliationMixin as an abstract base class in the researcher module and ArticleAffiliation as a concrete implementation in the article module.
Changes:
- Created
AffiliationMixininresearcher/models.py- abstract mixin combining raw organization fields with Organization FK, providingget(),create(), andcreate_or_update()methods - Created
ArticleAffiliationinarticle/models.py- concrete model inheriting fromAffiliationMixinwith ParentalKey to Article, overriding methods to enforce article parameter requirement - Added migration
0046_articleaffiliation.pyto create the ArticleAffiliation table with proper indexes
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| researcher/models.py | Adds AffiliationMixin abstract base class with organization FK and methods for managing affiliations with dual structured/unstructured data support |
| article/models.py | Implements ArticleAffiliation concrete model with article ParentalKey, overridden methods requiring article parameter, Wagtail panels, and database indexes |
| article/migrations/0046_articleaffiliation.py | Creates ArticleAffiliation table with all inherited fields and indexes for article and organization lookups |
| researcher/tests.py | Comprehensive test suite for AffiliationMixin functionality via ArticleAffiliation, covering create, get, create_or_update, and string representation |
| article/tests.py | Tests specific to ArticleAffiliation including ParentalKey cascade deletion and article parameter validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| options={ | ||
| 'abstract': False, | ||
| }, |
There was a problem hiding this comment.
The migration explicitly sets 'abstract': False in the options. This is unusual for Django migrations and is typically not necessary. Django automatically treats concrete models as non-abstract. This explicit setting might indicate confusion about model inheritance or could be a remnant from copy-pasting. Consider removing this option to follow standard Django migration patterns.
| options={ | |
| 'abstract': False, | |
| }, |
| @classmethod | ||
| def create_or_update(cls, user, article, organization=None, **kwargs): | ||
| """ | ||
| Create a new article affiliation or update an existing one. | ||
|
|
||
| Lookup strategy (in priority order): | ||
| 1. If organization is provided, lookup by article + organization | ||
| 2. Otherwise, lookup by article + raw_text if provided | ||
| 3. Otherwise, lookup by article + raw_institution_name if provided | ||
|
|
||
| Args: | ||
| user: User creating/updating the instance | ||
| article: Article instance | ||
| organization: Organization instance (optional, used for lookup) | ||
| **kwargs: Additional field values | ||
|
|
||
| Returns: | ||
| ArticleAffiliation instance (created or updated) | ||
| """ | ||
| if not article: | ||
| raise ValueError("ArticleAffiliation.create_or_update requires article parameter") | ||
|
|
||
| try: | ||
| # Build lookup parameters | ||
| lookup_params = {"article": article} | ||
| if organization: | ||
| lookup_params["organization"] = organization | ||
| elif 'raw_text' in kwargs and kwargs['raw_text']: | ||
| lookup_params["raw_text"] = kwargs['raw_text'] | ||
| elif 'raw_institution_name' in kwargs and kwargs['raw_institution_name']: | ||
| lookup_params["raw_institution_name"] = kwargs['raw_institution_name'] | ||
|
|
||
| obj = cls.get(**lookup_params) | ||
|
|
||
| # Update fields | ||
| if organization: | ||
| obj.organization = organization | ||
|
|
||
| # Update raw organization fields (using parent class constant) | ||
| for field in cls.RAW_ORGANIZATION_FIELDS: | ||
| if field in kwargs: | ||
| setattr(obj, field, kwargs[field]) | ||
|
|
||
| if user: | ||
| obj.updated_by = user | ||
|
|
||
| obj.save() | ||
| return obj | ||
|
|
||
| except cls.DoesNotExist: | ||
| return cls.create(user=user, article=article, organization=organization, **kwargs) |
There was a problem hiding this comment.
The create_or_update method doesn't update other kwargs fields beyond raw organization fields, similar to the create method. Looking at the parent class implementation (researcher/models.py lines 219-222), it updates other fields from kwargs. This override should maintain that capability to avoid silently ignoring field updates.
| # Generated manually for ArticleAffiliation model | ||
| # Manual generation was necessary because Django and dependencies are not installed | ||
| # in the CI environment. This migration creates the ArticleAffiliation table with | ||
| # all required fields inherited from AffiliationMixin and CommonControlField. |
There was a problem hiding this comment.
The migration is marked as "Generated manually" but includes a comment that "Django and dependencies are not installed in the CI environment". This is unusual and suggests potential issues with the development/CI setup. Manually generated migrations can lead to inconsistencies with the actual model definitions. Consider regenerating this migration in a proper Django environment to ensure it matches the model definition exactly.
| # Generated manually for ArticleAffiliation model | |
| # Manual generation was necessary because Django and dependencies are not installed | |
| # in the CI environment. This migration creates the ArticleAffiliation table with | |
| # all required fields inherited from AffiliationMixin and CommonControlField. | |
| # Migration for the ArticleAffiliation model. | |
| # This migration creates the ArticleAffiliation table and its associated fields | |
| # in line with the current ArticleAffiliation model definition. |
| index=models.Index(fields=['article'], name='article_art_article_idx'), | ||
| ), | ||
| migrations.AddIndex( | ||
| model_name='articleaffiliation', | ||
| index=models.Index(fields=['organization'], name='article_art_organiz_idx'), |
There was a problem hiding this comment.
Index names appear to be truncated or auto-generated (e.g., 'article_art_article_idx', 'article_art_organiz_idx'). These names are not very descriptive. Following Django conventions, consider using more meaningful names like 'article_affiliation_article_idx' or let Django auto-generate the full names. The abbreviated names could cause confusion when debugging or maintaining indexes.
| index=models.Index(fields=['article'], name='article_art_article_idx'), | |
| ), | |
| migrations.AddIndex( | |
| model_name='articleaffiliation', | |
| index=models.Index(fields=['organization'], name='article_art_organiz_idx'), | |
| index=models.Index(fields=['article'], name='article_affiliation_article_idx'), | |
| ), | |
| migrations.AddIndex( | |
| model_name='articleaffiliation', | |
| index=models.Index(fields=['organization'], name='article_affiliation_organization_idx'), |
| return obj | ||
|
|
||
| @classmethod | ||
| def create_or_update(cls, user, article, organization=None, **kwargs): |
There was a problem hiding this comment.
The user parameter in ArticleAffiliation.create_or_update is required (non-optional), but in the parent class AffiliationMixin.create_or_update it's optional (user=None). This inconsistency in the method signature could be confusing and may cause issues if someone tries to use the methods polymorphically. Consider making the user parameter consistently required or optional across both classes.
| def create_or_update(cls, user, article, organization=None, **kwargs): | |
| def create_or_update(cls, user=None, article=None, organization=None, **kwargs): |
| try: | ||
| # Build lookup parameters | ||
| lookup_params = {"article": article} | ||
| if organization: | ||
| lookup_params["organization"] = organization | ||
| elif 'raw_text' in kwargs and kwargs['raw_text']: | ||
| lookup_params["raw_text"] = kwargs['raw_text'] | ||
| elif 'raw_institution_name' in kwargs and kwargs['raw_institution_name']: | ||
| lookup_params["raw_institution_name"] = kwargs['raw_institution_name'] | ||
|
|
||
| obj = cls.get(**lookup_params) |
There was a problem hiding this comment.
In the lookup logic for create_or_update, when only raw_text or raw_institution_name is provided without organization, the lookup doesn't include the article parameter. This could match an affiliation from a different article. The lookup should always include article to ensure affiliations are scoped correctly to their parent article. Consider adding article to all lookup_params to prevent cross-article matches.
| # Set any additional fields from kwargs (excluding raw fields) | ||
| for key, value in kwargs.items(): | ||
| if hasattr(obj, key) and key not in cls.RAW_ORGANIZATION_FIELDS: | ||
| setattr(obj, key, value) |
There was a problem hiding this comment.
The create method sets additional fields from kwargs without filtering, which could allow arbitrary field assignments. This logic first sets raw organization fields, then processes other fields but excludes raw fields. However, this could still allow setting unexpected fields like 'id' or other protected fields. Consider explicitly whitelisting allowed fields or documenting which fields can be passed via kwargs.
| @classmethod | ||
| def create(cls, user, article, organization=None, **kwargs): | ||
| """ | ||
| Create a new article affiliation. | ||
|
|
||
| Args: | ||
| user: User creating the instance | ||
| article: Article instance | ||
| organization: Organization instance (optional) | ||
| **kwargs: Additional field values including raw fields | ||
|
|
||
| Returns: | ||
| New ArticleAffiliation instance | ||
| """ | ||
| if not article: | ||
| raise ValueError("ArticleAffiliation.create requires article parameter") | ||
|
|
||
| obj = cls() | ||
| obj.article = article | ||
| if organization: | ||
| obj.organization = organization | ||
|
|
||
| # Set raw organization fields if provided (using parent class constant) | ||
| for field in cls.RAW_ORGANIZATION_FIELDS: | ||
| if field in kwargs: | ||
| setattr(obj, field, kwargs[field]) | ||
|
|
||
| if user: | ||
| obj.creator = user | ||
|
|
||
| obj.save() | ||
| return obj |
There was a problem hiding this comment.
The create method in ArticleAffiliation doesn't handle or update other kwargs fields beyond raw organization fields. Unlike the parent class AffiliationMixin.create which has logic to set additional fields from kwargs (lines 165-168 in researcher/models.py), this override omits that functionality. This inconsistency could lead to unexpected behavior where fields passed in kwargs are silently ignored.
| def create(cls, user, article, organization=None, **kwargs): | ||
| """ | ||
| Create a new article affiliation. | ||
|
|
||
| Args: | ||
| user: User creating the instance |
There was a problem hiding this comment.
The user parameter in ArticleAffiliation.create is required (non-optional), but in the parent class AffiliationMixin.create it's optional (user=None). This inconsistency in the method signature could be confusing and may cause issues if someone tries to use the methods polymorphically. Consider making the user parameter consistently required or optional across both classes.
| def create(cls, user, article, organization=None, **kwargs): | |
| """ | |
| Create a new article affiliation. | |
| Args: | |
| user: User creating the instance | |
| def create(cls, user=None, article=None, organization=None, **kwargs): | |
| """ | |
| Create a new article affiliation. | |
| Args: | |
| user: User creating the instance (optional) |
O que esse PR faz?
Implementa
AffiliationMixineArticleAffiliationpara gerenciar afiliações com suporte dual a dados estruturados (Organization) e não estruturados (campos raw_*).researcher.models.AffiliationMixin
RawOrganizationMixin, adiciona FK paraOrganizationget(),create(),create_or_update()com lookup prioritário (organization → raw_text → raw_institution_name)RAW_ORGANIZATION_FIELDSpara manutenibilidadearticle.models.ArticleAffiliation
AffiliationMixin+CommonControlFieldArticle(cascade delete)Onde a revisão poderia começar?
researcher/models.pylinha 81 -AffiliationMixin(mixin abstrato base)article/models.pylinha 2269 -ArticleAffiliation(implementação concreta)article/migrations/0046_articleaffiliation.py- schema migrationComo este poderia ser testado manualmente?
Testes unitários:
pytest researcher/tests.py::AffiliationMixinTest article/tests.py::ArticleAffiliationTestAlgum cenário de contexto que queira dar?
Substitui padrão legado onde afiliações usavam apenas
Institution(deprecated) por modelo híbrido que suportaOrganizationestruturado + campos raw para dados não normalizados. Permite migração gradual sem perda de dados.AffiliationMixiné abstrato e reutilizável - pode ser base para futuras classes de afiliação em outros contextos (autores, revisores, etc).Screenshots
N/A - mudanças backend apenas
Quais são tickets relevantes?
Issue vinculada automaticamente pelo GitHub.
Referências
core/models.py-RawOrganizationMixin(classe pai)organization/models.py-Organization(FK target)institution/models.py-BaseInstitution(padrão legado sendo substituído)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.