Batch with additional historical model fields #1248

NoelJames · 2023-09-08T02:24:17Z

Description

Adds ability to update history model attributes (custom ones) during a batch job.

Related Issue

Motivation and Context

Adding additional fields to historical models has been a useful approach to capturing some necessary data for me. However, I also have some operations that require bulk_update_with_history and bulk_create_with_history but these functions do not support these customized field value.

How Has This Been Tested?

The pr includes tests

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:

I have run the pre-commit run command to format and lint.
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.
I have added my name and/or github handle to AUTHORS.rst
I have added my change to CHANGES.rst
All new and existing tests passed.

NoelJames · 2023-09-08T14:16:06Z

I wanted to note a few things about this

I can not get the documents to generate on my machine or run the pre-commit run. For the docuemts i get a cryptic error that says docs: FAIL code 2 and i can not find the pre-commit command. The tests do run.

Maybe **kwargs is not the best. I could change to param named custom or additional and expect a dict that would almost be the same thing?

I did not add the pydoc to the function which had the signatures changed

let me know if this looks like something you'd go forward with and I'll make changes

codecov · 2023-09-08T22:31:49Z

Codecov Report

Merging #1248 (c1ec612) into master (bde32e1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1248   +/-   ##
=======================================
  Coverage   96.94%   96.94%           
=======================================
  Files          23       23           
  Lines        1275     1277    +2     
  Branches      209      210    +1     
=======================================
+ Hits         1236     1238    +2     
  Misses         21       21           
  Partials       18       18

Files	Coverage Δ
simple_history/manager.py	`97.36% <ø> (ø)`
simple_history/utils.py	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ddabble · 2023-09-14T17:02:58Z

For the docuemts i get a cryptic error that says docs: FAIL code 2

I wouldn't worry about this, as the docs are built through the CI anyway 🙂 (See the checks at the bottom of this page - which contain this link to the build for this PR: https://django-simple-history--1248.org.readthedocs.build/en/1248/.)

i can not find the pre-commit command.

This sounds like pre-commit is not installed on your machine 🤔 See pre-commit's website if that's the case. Otherwise, the command is also run through the CI, so you don't have to run it locally :)

Maybe **kwargs is not the best. I could change to param named custom or additional and expect a dict that would almost be the same thing?

Yeah, I agree. The custom_ prefixes of the passed kwargs are pretty unconventional, and so it has potential for causing confusion. I think it would indeed be better to replace the **kwargs like you mentioned, but preferably name the parameter something a little more explicit, like extra_historical_attrs/custom_historical_attrs.

I did not add the pydoc to the function which had the signatures changed

It would be great if you could update the docstrings of bulk_create_with_history() and bulk_update_with_history() 🙂

let me know if this looks like something you'd go forward with and I'll make changes

Sure, this absolutely seems useful! Thank you for taking the time 😊

ddabble

Some additional changes to the ones discussed above:

simple_history/tests/tests/test_utils.py

CHANGES.rst

docs/common_issues.rst

simple_history/manager.py

NoelJames · 2023-09-25T16:10:36Z

I've submitted the suggested changes.

NoelJames · 2023-09-25T17:57:45Z

the failing tests on Django main with AttributeError: 'BulkHistoryUpdateTestCase' object has no attribute 'assertQuerysetEqual'. Did you mean: 'assertQuerySetEqual'? are not mine

ddabble

Nice! Some feedback on the new code:

CHANGES.rst

simple_history/tests/tests/test_utils.py

simple_history/manager.py

docs/common_issues.rst

ddabble · 2023-09-25T19:30:01Z

the failing tests on Django main with AttributeError: 'BulkHistoryUpdateTestCase' object has no attribute 'assertQuerysetEqual'. Did you mean: 'assertQuerySetEqual'? are not mine

Yeah, it's caused by some recent changes in Django's main branch, like you mentioned. The tests were fixed in #1258, so if you want, you can rebase on master, but I'll merge in any case :)

NoelJames · 2023-09-25T19:51:52Z

Alright, I'll ~~update master~~ my pr fix the doc string and then I'll take the PR out of draft when its ready.

Thanks for your help

ddabble · 2023-09-25T21:52:40Z

I noticed that the bulk_update_with_history() example was missing a value for the fields argument, and that the test case for the same function was providing a field name that was logically unnecessary - as no fields were actually updated on the model - so I pushed commit 42c59aa :)

ddabble

Great job, thank you! 😊

ddabble · 2023-09-25T22:02:49Z

@NoelJames Lastly, before merging, would you like to rebase to clean up your commits? 🙂 Otherwise, I'll just squash them, as to me they seem to be telling one coherent story (except for 42c59aa, I guess).

NoelJames · 2023-09-25T22:09:00Z

I got pretty messy there. And rebase has not been very good to me so, please squash away.

ddabble · 2023-09-25T22:19:28Z

No worries, it's just natural when working on a PR after feedback :) But alright 👌

* docs * add history attribute to batch * doc updates * uses `custom_historical_attrs` not kwargs * updates docs with PollWithHistoricalSessionAttr and session attr * changes example values to not be same as other examples * adds PollWithHistoricalSessionAttr for testing * updates test for PollWithHistoricalSessionAttr * adds custom_historical_attrs to test_bulk_create_no_ids_return test * corrected change list * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * updates `SessionsHistoricalModel` with explicit default=None * updates CustomHistoricalAttrsTest test to use TestCase setUp * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fixes: id value can not be 0 * Update CHANGES.rst Co-authored-by: Anders <[email protected]> * Update simple_history/tests/tests/test_utils.py Co-authored-by: Anders <[email protected]> * Update docs/common_issues.rst Co-authored-by: Anders <[email protected]> * Update docs/common_issues.rst Co-authored-by: Anders <[email protected]> * Update docs/common_issues.rst Co-authored-by: Anders <[email protected]> * Update docs/common_issues.rst Co-authored-by: Anders <[email protected]> * Update simple_history/tests/tests/test_utils.py Co-authored-by: Anders <[email protected]> * Update simple_history/tests/tests/test_utils.py Co-authored-by: Anders <[email protected]> * Update simple_history/tests/tests/test_utils.py Co-authored-by: Anders <[email protected]> * Update simple_history/tests/tests/test_utils.py Co-authored-by: Anders <[email protected]> * Update simple_history/tests/tests/test_utils.py Co-authored-by: Anders <[email protected]> * Update simple_history/manager.py Co-authored-by: Anders <[email protected]> * updates docstring --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Anders <[email protected]>

This fixes a "hack" for when a user only wants to provide the `custom_historical_attrs` argument, where a list with arbitrary field names could be provided as a circumvention for the error that was raised otherwise - even if no model fields were actually changed.

ddabble requested changes Sep 14, 2023

View reviewed changes

NoelJames closed this Sep 25, 2023

NoelJames force-pushed the master branch from 234f2a9 to aa87e7c Compare September 25, 2023 15:51

NoelJames reopened this Sep 25, 2023

ddabble requested changes Sep 25, 2023

View reviewed changes

NoelJames marked this pull request as draft September 25, 2023 19:50

NoelJames marked this pull request as ready for review September 25, 2023 20:06

ddabble approved these changes Sep 25, 2023

View reviewed changes

NoelJames and others added 2 commits September 26, 2023 00:31

ddabble force-pushed the master branch from 42c59aa to c1ec612 Compare September 25, 2023 22:32

ddabble merged commit 329659b into jazzband:master Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch with additional historical model fields #1248

Batch with additional historical model fields #1248

NoelJames commented Sep 8, 2023

NoelJames commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

ddabble commented Sep 14, 2023

ddabble left a comment

NoelJames commented Sep 25, 2023

NoelJames commented Sep 25, 2023

ddabble left a comment

ddabble commented Sep 25, 2023

NoelJames commented Sep 25, 2023 •

edited

Loading

ddabble commented Sep 25, 2023

ddabble left a comment

ddabble commented Sep 25, 2023

NoelJames commented Sep 25, 2023

ddabble commented Sep 25, 2023

Batch with additional historical model fields #1248

Batch with additional historical model fields #1248

Conversation

NoelJames commented Sep 8, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

NoelJames commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

ddabble commented Sep 14, 2023

ddabble left a comment

Choose a reason for hiding this comment

NoelJames commented Sep 25, 2023

NoelJames commented Sep 25, 2023

ddabble left a comment

Choose a reason for hiding this comment

ddabble commented Sep 25, 2023

NoelJames commented Sep 25, 2023 • edited Loading

ddabble commented Sep 25, 2023

ddabble left a comment

Choose a reason for hiding this comment

ddabble commented Sep 25, 2023

NoelJames commented Sep 25, 2023

ddabble commented Sep 25, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

NoelJames commented Sep 25, 2023 •

edited

Loading