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

Feature Request: Add added_date Field to OTP Device Models for Enhanced Audit and Management #137

Closed
rushikeshiam opened this issue Apr 4, 2024 · 3 comments

Comments

@rushikeshiam
Copy link
Contributor

rushikeshiam commented Apr 4, 2024

I propose the addition of an added_date field to the models within django_otp. This field would serve to track the date and time when an OTP device is added, enhancing the audit capabilities and management of OTP devices.

Suggested Implementation

Field Name: added_date
Field Type: DateTimeField
Attributes: auto_now_add=True

@psagers
Copy link
Member

psagers commented Apr 4, 2024

It's a reasonable idea, but as the CONTRIBUTING doc mentions, adding a model field to any of the abstract classes (Device included) is a complicated affair. Every plugin will need a migration; not just the built-in ones, but every third-party and custom plugin as well. A change like this would probably require a 2.0 release and then every plugin would need a major version release and every project would need to upgrade everything to mutually compatible versions at once.

Using abstract models may have been an infelicitous design decision at the outset, but that's where we are. And since this project is basically finished and stable, this is likely outside the bounds of what's possible.

Having said that, one thing that is entirely safe is adding new abstract model classes for new functionality. We've done that several times already. So if you wanted to add some kind of AuditableDevice mixin (better name welcome), then individual device classes could adopt the functionality at their leisure in a backward-compatible way. You'd want to think very carefully about what fields you might want in that, as once a device mixin is shipped, it can never be extended.

@rushikeshiam
Copy link
Contributor Author

@psagers
Thank you for your insightful feedback. Based on your suggestions, I’ve pivoted to introducing an AuditableMixin instead of modifying the base Device model. This approach minimizes impact on existing deployments and allows for optional, backward-compatible adoption of new audit features.

I've updated the PR to reflect these changes, ensuring it aligns with the project’s design principles and your advice for maintaining stability. I look forward to your thoughts on this revised contribution.

@psagers
Copy link
Member

psagers commented Apr 9, 2024

Fixed by #138/#139.

@psagers psagers closed this as completed Apr 9, 2024
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 a pull request may close this issue.

2 participants