Skip stringify if json #355

barrachri · 2022-02-08T15:31:54Z

Fixes #354

Linkid

Thanks for your contribution. I let you some comments.

auditlog/diff.py

barrachri · 2022-03-31T06:59:09Z

Hi @Linkid, can you give the PR another check? Thanks!

Linkid

Hi! Sorry about that, but with the PR #370, django-jsonfield-backport is removed. Then, you can use this line instead:
from django.db.models import JSONField

alexsavio · 2022-05-08T20:25:10Z

Sounds good to me. Thanks for the review @Linkid. When are you planning to make a release, though? Thanks a lot!

auditlog/diff.py

for more information, see https://pre-commit.ci

codecov · 2022-05-08T21:28:53Z

Codecov Report

Merging #355 (a7e5a7f) into master (d848c53) will increase coverage by 0.75%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   82.64%   83.39%   +0.75%     
==========================================
  Files          20       20              
  Lines         507      506       -1     
==========================================
+ Hits          419      422       +3     
+ Misses         88       84       -4

Impacted Files	Coverage Δ
auditlog/diff.py	`94.11% <100.00%> (+5.71%)`	⬆️

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 d848c53...a7e5a7f. Read the comment docs.

* master: Remove default_app_config configuration Fix lint entry in tox.ini Drop Django 2.2 support. [pre-commit.ci] pre-commit autoupdate Fix small typo Add mask_fields argument in register (jazzband#310) Use pre-commit as lint command Add black and isort to .pre-commit-config.yaml Replace assertTrue with assertEqual enable use of replica database (jazzband#359)

hramezani · 2022-05-09T10:09:22Z

Thanks @barrachri for the patch. Please:

Rebase your patch
Add regression tests to prove the patch.

* master: Prepare release of django-auditlog 2.0.0 Update postgres image version in github action test workflow Fix typo in CHANGELOG.md

barrachri · 2022-05-09T14:13:03Z

Thanks @hramezani. Do you think the added tests are enough?

hramezani · 2022-05-09T14:49:27Z

Thanks @barrachri for the tests. But I can see your tests are passing without the fix.
You need to add a test that shows the problem which your patch fixes it.

Also, Please add a changelog entry.

barrachri · 2022-05-09T15:59:07Z

Hi @hramezani you can try to run it again, while disabling the elif isinstance(field, JSONField): if branch.

I also added the changelog entry.

CHANGELOG.md

Co-authored-by: Hasan Ramezani <[email protected]>

hramezani · 2022-05-09T16:48:01Z

auditlog/diff.py

+    elif isinstance(field, JSONField):
+        try:
+            value = field.to_python(getattr(obj, field.name, None))
+        except ObjectDoesNotExist:


Do we have a test that covers this except block 🤔 ?

There's no test, in general, that covers the except ObjectDoesNotExist: part of get_field_value.

I am happy to add one though not entirely sure how to replicate that directly with the model (I believe we would need a migration to replicate it with save()).

But I can call model_instance_diff directly, with 2 instances and one has the attribute deleted.

BTW, it is a blocker because coverage fails and I can't merge the PR

…into fix-jsonfield * 'fix-jsonfield' of github.com:barrachri/django-auditlog: Update CHANGELOG.md Update CHANGELOG.md

hramezani

Great work @barrachri 👍

barrachri · 2022-05-09T19:03:13Z

Thanks @hramezani, I added a new test and refactored the ObjectDoesNotExist try/except.

Problem fixed

Skip stringify if json

df22190

Linkid requested changes Mar 1, 2022

View reviewed changes

auditlog/diff.py Show resolved Hide resolved

auditlog/diff.py Show resolved Hide resolved

Fix after PR

3fc4cf3

barrachri requested a review from Linkid April 5, 2022 23:29

Linkid previously requested changes May 1, 2022

View reviewed changes

barrachri commented May 8, 2022

View reviewed changes

auditlog/diff.py Outdated Show resolved Hide resolved

barrachri and others added 2 commits May 8, 2022 23:27

Update auditlog/diff.py

15f1d41

[pre-commit.ci] auto fixes from pre-commit.com hooks

aaec783

for more information, see https://pre-commit.ci

barrachri added 4 commits May 9, 2022 12:33

Merge branch 'master' into fix-jsonfield

055dd0c

* master: Prepare release of django-auditlog 2.0.0 Update postgres image version in github action test workflow Fix typo in CHANGELOG.md

Add tests

152e020

Add tests

efaf4b6

Add better comment

edbf59b

barrachri added 2 commits May 9, 2022 17:52

Show test can fail

10c5741

Add entry to changelog

c3fcbfc

barrachri added 2 commits May 9, 2022 18:00

Fix typo

bd2673c

Improve test clarity

319dfa7

hramezani reviewed May 9, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

hramezani reviewed May 9, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

barrachri and others added 2 commits May 9, 2022 18:16

Update CHANGELOG.md

0afa1fc

Co-authored-by: Hasan Ramezani <[email protected]>

Update CHANGELOG.md

41d789a

Co-authored-by: Hasan Ramezani <[email protected]>

hramezani reviewed May 9, 2022

View reviewed changes

Remove old backport

5112258

barrachri added 2 commits May 9, 2022 19:14

Merge branch 'fix-jsonfield' of github.com:barrachri/django-auditlog …

a68abe3

…into fix-jsonfield * 'fix-jsonfield' of github.com:barrachri/django-auditlog: Update CHANGELOG.md Update CHANGELOG.md

Remove breakpoints

dda2b78

hramezani previously approved these changes May 9, 2022

View reviewed changes

Add test to cover ObjectDoesNotExist

23fae45

barrachri dismissed hramezani’s stale review via 23fae45 May 9, 2022 19:02

barrachri added 2 commits May 9, 2022 21:05

Fix docstring

be16e22

Fix test

a7e5a7f

hramezani approved these changes May 10, 2022

View reviewed changes

hramezani merged commit a93f539 into jazzband:master May 10, 2022

barrachri deleted the fix-jsonfield branch May 10, 2022 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip stringify if json #355

Skip stringify if json #355

barrachri commented Feb 8, 2022

Linkid left a comment

barrachri commented Mar 31, 2022

Linkid left a comment

alexsavio commented May 8, 2022

codecov bot commented May 8, 2022 •

edited

Loading

hramezani commented May 9, 2022

barrachri commented May 9, 2022

hramezani commented May 9, 2022

barrachri commented May 9, 2022

hramezani May 9, 2022

barrachri May 9, 2022 •

edited

Loading

hramezani May 9, 2022

hramezani left a comment

barrachri commented May 9, 2022 •

edited

Loading

Skip stringify if json #355

Skip stringify if json #355

Conversation

barrachri commented Feb 8, 2022

Linkid left a comment

Choose a reason for hiding this comment

barrachri commented Mar 31, 2022

Linkid left a comment

Choose a reason for hiding this comment

alexsavio commented May 8, 2022

codecov bot commented May 8, 2022 • edited Loading

Codecov Report

hramezani commented May 9, 2022

barrachri commented May 9, 2022

hramezani commented May 9, 2022

barrachri commented May 9, 2022

hramezani May 9, 2022

Choose a reason for hiding this comment

barrachri May 9, 2022 • edited Loading

Choose a reason for hiding this comment

hramezani May 9, 2022

Choose a reason for hiding this comment

hramezani left a comment

Choose a reason for hiding this comment

barrachri commented May 9, 2022 • edited Loading

codecov bot commented May 8, 2022 •

edited

Loading

barrachri May 9, 2022 •

edited

Loading

barrachri commented May 9, 2022 •

edited

Loading