add people model #1227
add people model #1227
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @alphatownsman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the catalog's data model by introducing a dedicated system for managing people and organizations. It allows for rich, role-based associations between catalog items and these entities, improving the depth and interconnectedness of the data. The changes are designed to seamlessly integrate with existing item management functionalities, particularly item merging, to ensure consistent and accurate data representation.
Highlights
- Introduction of new 'People' model: A new
Peoplemodel has been added, inheriting from the existingItemmodel. This allows for a structured representation of individuals and organizations within the catalog, with support for distinguishing between 'person' and 'organization' types. - Implementation of 'ItemPeopleRelation' for linking: A new 'through' model,
ItemPeopleRelation, has been introduced to establish many-to-many relationships betweenItemsandPeople. This model includes arolefield, allowing specific associations like 'author', 'director', 'publisher', and acharacterfield for actor roles. - Enhanced 'Item' model with people relation management: The core
Itemmodel has been updated to include methods for managing and mergingItemPeopleRelationinstances. This ensures that whenItemobjects are merged, their associatedPeoplerelations are correctly transferred and duplicates are handled, maintaining data integrity. - Comprehensive unit testing for new models: Extensive unit tests have been added to cover the creation, merging, deletion, and relation management functionalities of the new
PeopleandItemPeopleRelationmodels, ensuring robustness and correctness.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new People model, inheriting from Item, to represent people and organizations, along with a through model ItemPeopleRelation to link them to other items with specific roles. The changes are well-structured and include comprehensive tests. My review focuses on a few critical issues: a missing database field in the migration, a bug in the item merge logic that could lead to data loss, and an opportunity to improve exception handling for better robustness. Addressing these points will solidify this excellent contribution.
| if relation.metadata and not existing_relation.metadata: | ||
| existing_relation.metadata = relation.metadata | ||
| existing_relation.save() |
There was a problem hiding this comment.
The logic for merging an existing ItemPeopleRelation only considers the metadata field. It's missing the logic to merge the character field. The new test test_item_merge_with_duplicate_people_relations correctly expects the character information to be preserved, but it will fail with the current implementation. The merge logic should be updated to handle both character and metadata to avoid data loss on merge.
| if relation.metadata and not existing_relation.metadata: | |
| existing_relation.metadata = relation.metadata | |
| existing_relation.save() | |
| relation_updated = False | |
| if relation.character and not existing_relation.character: | |
| existing_relation.character = relation.character | |
| relation_updated = True | |
| if relation.metadata and not existing_relation.metadata: | |
| existing_relation.metadata = relation.metadata | |
| relation_updated = True | |
| if relation_updated: | |
| existing_relation.save() |
| class ItemPeopleRelation(models.Model): | ||
| """Through model linking Items to People with roles""" | ||
|
|
||
| item = models.ForeignKey( | ||
| Item, on_delete=models.CASCADE, related_name="people_relations" | ||
| ) | ||
| people = models.ForeignKey( | ||
| People, on_delete=models.CASCADE, related_name="item_relations" | ||
| ) | ||
| role = models.CharField(_("role"), max_length=50, choices=PeopleRole.choices) | ||
| character = jsondata.CharField( | ||
| _("character"), | ||
| max_length=1000, | ||
| null=True, | ||
| blank=True, | ||
| help_text=_("Character name for actor roles"), | ||
| ) | ||
| metadata = models.JSONField(_("metadata"), blank=True, null=True, default=dict) | ||
| created_time = models.DateTimeField(auto_now_add=True) | ||
| edited_time = models.DateTimeField(auto_now=True) | ||
|
|
||
| class Meta: | ||
| unique_together = [["item", "people", "role"]] | ||
| indexes = [ | ||
| models.Index(fields=["item", "role"]), | ||
| ] |
There was a problem hiding this comment.
The ItemPeopleRelation model correctly includes a character field, which is useful for roles like 'Actor'. However, the corresponding migration file catalog/migrations/0003_people_itempeoplerelation.py is missing this field. This will cause an OperationalError: no such column when the application tries to access this field. Please remember to generate and include a new migration to add the character field to the database schema.
catalog/models/people.py
Outdated
| "resolve merge loop error for people", extra={"people": people} | ||
| ) | ||
| people = None | ||
| except Exception: |
There was a problem hiding this comment.
The try...except block uses a broad except Exception:, which can hide unexpected errors and make debugging difficult. It's a best practice to catch specific exceptions that are expected to occur, such as cls.DoesNotExist and ValueError (from b62_decode). This makes the code more robust and easier to maintain.
| except Exception: | |
| except (cls.DoesNotExist, ValueError, TypeError): |
1 participant
No description provided.