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 new settings attr AUDITLOG_REGISTRY to allow custom auditlogs #623

Conversation

hoangquochung1110
Copy link
Contributor

@hoangquochung1110 hoangquochung1110 commented Apr 2, 2024

This PR is to close #617

I've added a new configuration AUDITLOG_REGISTRY, paths to custom auditlog instances
which allow to configure models logging with flexible options (conditionally disabled/enabled logging and custom signal receivers)

@hoangquochung1110 hoangquochung1110 force-pushed the add-custom-auditlog branch 2 times, most recently from 0e4e717 to 96c4355 Compare April 2, 2024 07:56
@hoangquochung1110 hoangquochung1110 marked this pull request as draft April 2, 2024 09:04
@hoangquochung1110 hoangquochung1110 force-pushed the add-custom-auditlog branch 2 times, most recently from e036e36 to 1c6bbf6 Compare April 2, 2024 09:36
@hoangquochung1110 hoangquochung1110 marked this pull request as ready for review April 2, 2024 09:38
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 95.11%. Comparing base (5289482) to head (7a52038).
Report is 4 commits behind head on master.

Files Patch % Lines
auditlog/registry.py 90.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
- Coverage   95.21%   95.11%   -0.11%     
==========================================
  Files          31       31              
  Lines        1025     1064      +39     
==========================================
+ Hits          976     1012      +36     
- Misses         49       52       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hramezani
Copy link
Member

Thanks @hoangquochung1110 for the PR.

Honestly, the implementation looks complex to me. I was expecting a simple config that can be used instead of the default auditlog instance and I would suggest to have this for now.

BTW, if you are interested in implementing multiple auditlog instances at the same time, You can have a registry class to map every model to its auditlog instance. you can build the mapping in register method. then you can have multiple auditlog instance at the same time and the instances can be used for models registrations. something like:

from auditlog.registry import auditlog  # default one

custom_auditlog = AuditlogModelRegistry(create=False)

# here in `register` method we have to map the `MyModel` to default auditlog instance
auditlog.register(MyModel)

# here in `register` method we have to map the `MyModel1` to `custom_auditlog`
custom_auditlog.register(MyModel1)

Then in the other places of project, we have to find the auditlog instance based on model

@hoangquochung1110
Copy link
Contributor Author

Thanks @hramezani for the review

I realize the implementation looks unnecessarily complex.

My initial goal is to disable signals and define custom signal handlers

I dived into the documentations
and the below snippet of code works for my case:

auditlog.register(
    Model,
)
pre_log.connect(lambda sender, **kwargs: False if sender == Model else True)
post_save.connect(
    receiver=lambda *args, **kwargs: log_create_reverse_fk_to_opportunity(
            action=custom_action, *args, **kwargs,
    ),
    sender=model,
)

I'll close the PR and the issue soon. Thanks

@hramezani
Copy link
Member

Thanks @hoangquochung1110 for letting me know

@hramezani hramezani closed this Apr 8, 2024
@CleitonDeLima
Copy link
Contributor

Hello,
I was thinking, if AuditlogModelRegistry was a singleton, could this idea work for multiple instances?

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.

Have custom AuditLogModelRegistry
3 participants