Skip to content

Conversation

@lmiguelvargasf
Copy link
Contributor

Description

  1. Add '+' as the history_type for each instance in bulk_history_create
  2. Add support for history_change_reason for each instance in bulk_history_create

Related Issue

#442

Motivation and Context

This solves issue #442

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

CHANGES.rst Outdated
----------
- Add ability to cascade delete historical records when master record is deleted
- Add ability to cascade delete historical records when master record is deleted
- Add `'+'` as the `history_type` for each instance in `bulk_history_create`

Choose a reason for hiding this comment

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

Just add this PR number next to the change please. i.e. (gh-449)

>>> for poll in data:
poll.changeReason = 'reason'
>>> objs = bulk_create_with_history(data, Poll, batch_size=500)
>>> Poll.objects.get(id=data[0].id).history_change_reason

Choose a reason for hiding this comment

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

Wouldn't this have to be HistoricalPoll or Poll.history.get? I don't think there's a history_change_reason on the base instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rossmechanic , yeah you are right.

self.model(
history_date=getattr(instance, '_history_date', now()),
history_user=getattr(instance, '_history_user', None),
history_change_reason=getattr(instance, 'changeReason', ''),

Choose a reason for hiding this comment

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

👍

@rossmechanic
Copy link

Tests failed, which is why #447 will be awesome. Just a few questions around documentation of changeReason but overall lgtm

@rossmechanic rossmechanic added the DjangoConUS2018 Work resulting from DjangoConUS2018 sprint label Oct 18, 2018
@codecov-io
Copy link

codecov-io commented Oct 18, 2018

Codecov Report

Merging #449 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #449   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files          15       15           
  Lines         679      679           
  Branches       93       93           
=======================================
  Hits          661      661           
  Misses          9        9           
  Partials        9        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f975c4...3d34040. Read the comment docs.

Copy link
Contributor

@barm barm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

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

lgtm

@rossmechanic rossmechanic merged commit 882ff36 into django-commons:master Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DjangoConUS2018 Work resulting from DjangoConUS2018 sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants