Skip to content

Conversation

@weilu
Copy link
Contributor

@weilu weilu commented Nov 15, 2025

This allows for efficient update and create of multiple instances based on 'id' field – those without supplied id values are created, otherwise updated.
All operations are atomic - either all succeed or all fail.

Unit tests in social_protection module due to dependency requirements

Unit tests in social_protection module due to dependency requirements
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a bulk_save class method to the HistoryModel for efficiently creating and updating multiple instances in a single atomic transaction. Instances without an id are created, while those with an id are updated.

  • Adds bulk_save class method supporting batch operations with configurable batch size
  • Implements atomic transaction handling to ensure all-or-nothing semantics
  • Handles version tracking and timestamp management for both create and update operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +282 to +283
update_fields = [f for f in to_update[0].__dict__.keys()
if not f.startswith('_') and f not in exclude_fields]
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

Accessing to_update[0] without checking if to_update is empty will raise an IndexError. This code is inside the if to_update: block but relies on at least one element existing. While the if to_update: check prevents the block from executing when empty, the logic assumes all instances have identical field sets, which may not hold if the model has fields with null=True or if different subclasses are mixed. Consider extracting field names from the model's _meta.get_fields() instead of relying on instance __dict__.

Suggested change
update_fields = [f for f in to_update[0].__dict__.keys()
if not f.startswith('_') and f not in exclude_fields]
update_fields = [
field.name for field in cls._meta.get_fields()
if (
field.concrete
and not field.auto_created
and not (field.many_to_many or field.one_to_many)
and field.name not in exclude_fields
)
]

Copilot uses AI. Check for mistakes.
return new_instance

@classmethod
def bulk_save(cls, data_list, user, batch_size=100):
Copy link
Member

Choose a reason for hiding this comment

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

can it be default at None and retrieve it from the get_current_user() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Models shouldn't be aware of current user as that's at the controller layer.

to ensure historical records are created correctly
Copy link
Contributor

@zikani03 zikani03 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Given a project, the user can view and modify the associated timesheet

4 participants