Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to globally exclude fields by name on all models #498

Conversation

darkpixel
Copy link
Contributor

My first stab at solving #467.

Adds the ability to list field names in AUDITLOG_EXCLUDE_TRACKING_MODELS that will be excluded from logging.
If a field in AUDITLOG_EXCLUDE_TRACKING_MODELS is the only field changed, nothing will be logged.

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #498 (b7895bb) into master (7a7e2eb) will decrease coverage by 0.10%.
The diff coverage is 66.66%.

❗ Current head b7895bb differs from pull request most recent head bb1979d. Consider uploading reports for the commit bb1979d to get more accurate results

@@            Coverage Diff             @@
##           master     #498      +/-   ##
==========================================
- Coverage   94.16%   94.07%   -0.10%     
==========================================
  Files          30       30              
  Lines         891      894       +3     
==========================================
+ Hits          839      841       +2     
- Misses         52       53       +1     
Impacted Files Coverage Δ
auditlog/registry.py 97.93% <50.00%> (-0.68%) ⬇️
auditlog/conf.py 100.00% <100.00%> (ø)

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

@hramezani
Copy link
Member

Thanks @darkpixel for this patch.

You need to consider:

  • Add a check for the value of this setting to be list or tuple like
    if not isinstance(settings.AUDITLOG_EXCLUDE_TRACKING_MODELS, (list, tuple)):
    raise TypeError(
    "Setting 'AUDITLOG_EXCLUDE_TRACKING_MODELS' must be a list or tuple"
    )
  • Add a check to only enable this setting with AUDITLOG_INCLUDE_ALL_MODELS like
    if (
    not settings.AUDITLOG_INCLUDE_ALL_MODELS
    and settings.AUDITLOG_EXCLUDE_TRACKING_MODELS
    ):
    raise ValueError(
    "In order to use setting 'AUDITLOG_EXCLUDE_TRACKING_MODELS', "
    "setting 'AUDITLOG_INCLUDE_ALL_MODELS' must set to 'True'"
  • Add some test for this change
  • Add a changelog entry in https://github.com/jazzband/django-auditlog/blob/master/CHANGELOG.md

"modified"
)

.. versionadded:: 2.2.3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. versionadded:: 2.2.3
.. versionadded:: 3.0.0

@darkpixel
Copy link
Contributor Author

Thanks @hramezani

I should be able to get those changed pushed later today!

@hramezani
Copy link
Member

Great @darkpixel
Please rebase your patch with master because the failing CI in Python 3.8 and 3.9 got fixed in 7a7e2eb

@darkpixel
Copy link
Contributor Author

The pre-commit hook that reformats code is a huge pain in the rear.

I made some changes, pushed, then continued working on test cases and went to push....and it was rejected because my fork/branch has changes. Spent 10 minutes trying to clean up/rebase a bunch of code the bot reformatted out from under me...thought I had it fixed, committed and pushed....and the bot changed things out from under me again. 😠

@darkpixel darkpixel force-pushed the feature/GH-467-AUDITLOG-EXCLUDE-TRACKING-FIELDS branch from 58685fa to 6a127d3 Compare January 19, 2023 17:13
@darkpixel
Copy link
Contributor Author

darkpixel commented Jan 19, 2023

Something's not making sense to me with the test framework.

I verified that the changes I've made actually work in my production application--fields listed in AUDITLOG_EXCLUDE_TRACKING_FIELDS = ('created', 'modified') prevents changes to those fields from being logged.

But given this test case, they are being logged:

class GlobalExcludeFieldTest(TestCase):
    """ Do not log changes to globally excluded fields. """

    def setUp(self):
        self.obj = SimpleModel.objects.create(text='Testing')

    def test_update_excluded_field(self):
        """ Test that updates to excluded fields are not logged """
        with override_settings(AUDITLOG_INCLUDE_ALL_MODELS=True) and override_settings(AUDITLOG_EXCLUDE_TRACKING_FIELDS=('datetime',)):
                obj = self.obj
                obj.datetime = datetime.datetime.now()
                obj.save()
                                                                                                                                                                                                                                                
                self.assertEqual(
                    obj.history.filter(action=LogEntry.Action.UPDATE).count(),
                    0,
                    msg="There are no log entries",
                )

I've poked through all the tests, and it seems like it should be working...but my guess is auditlog is already initialized with the wrong settings by the time my test starts?

@hramezani
Copy link
Member

@darkpixel Thanks for your update.

Your branch is protected and I am not allowed to push to your PR. Could you please allow me? I've fixed the problem and want to push.

@darkpixel
Copy link
Contributor Author

Could you please allow me? I've fixed the problem and want to push.

Added!

@hramezani
Copy link
Member

@darkpixel Thanks!

Please check my changes and if it is ok for you, I will merge it

@darkpixel
Copy link
Contributor Author

self.test_auditlog.register_from_settings()

Aah--that's what I was missing.

Do I need to go into any more depth on the tests? i.e. create a model, change an excluded field, and then check to make sure it wasn't saved as part of the changes dict?

@hramezani
Copy link
Member

No, I think it is enough to test it at this level. no need to go deeper

@hramezani hramezani merged commit 06ae048 into jazzband:master Jan 20, 2023
@hramezani hramezani deleted the feature/GH-467-AUDITLOG-EXCLUDE-TRACKING-FIELDS branch January 20, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants