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

chore: Renamed changed/created_on to changed/created_at #10719

Closed

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Aug 28, 2020

SUMMARY

As part of [SIP-27] Proposal for Paranoid Deletes with the addition of paranoid deletes it seems that, per the SIP, the sqla-paranoid package would be useful which provides the deleted_at column.

Currently for various audit tables have changed_on and created_on yet these refer to timestamps as opposed to dates and the tendency is to use _at for timestamps and _on for dates (example). To ensure consistency with sqla-paranoid I thought there would be merit in renaming the various changed_on and created_on columns to changed_at and created_at respectively.

I probably should have dug a little further before actioning on this PR as it seems that the auditability comes from FAB which explicitly defines the changed_on and created_on columns which are used in the ab_user table. I'm uncertain how people feel about this change. Note as a work around I needed to make the AuditMixinNullable not inherit from flask_appbuilder.models.mixins.AuditMixin.

Thoughts @bkyryliuk, @dpgaspar, @villebro, et al?

Note as a side note the mypy checks are failing when deprecating the flask_appbuilder.models.mixins.AuditMixin. Irrespective of whether we go ahead with said change it's highlighted a number of previously silenced (due to inheritance) typing issues which should be resolved.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI and manual testing.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley force-pushed the john-bodley--changed-created-at branch from 1c43cba to c9b5ef2 Compare August 28, 2020 18:47
@john-bodley john-bodley marked this pull request as ready for review August 28, 2020 18:47
@john-bodley john-bodley force-pushed the john-bodley--changed-created-at branch from c9b5ef2 to ed41bc4 Compare August 28, 2020 18:57
@john-bodley john-bodley force-pushed the john-bodley--changed-created-at branch from ed41bc4 to 8d8cb8a Compare August 28, 2020 20:05
@bkyryliuk
Copy link
Member

@john-bodley I just briefly looked into the https://github.com/jeanphix/sqla-paranoid and it feels to me that reimplementing it could be more straightforward vs migrating superset dbs to follow it's convention. There are only couple hundred lines.

@john-bodley
Copy link
Member Author

@bkyryliuk I agree it’s not much work to reimplement, though I do sense _at is more accurate than _on and thus adheres to other systems.

@john-bodley john-bodley force-pushed the john-bodley--changed-created-at branch from 8d8cb8a to 3bb0dd3 Compare August 28, 2020 22:36
@john-bodley
Copy link
Member Author

@dpgaspar what are your thoughts from a FAB perspective about introducing a breaking change which renames the audit columns from _on to _at?

@bkyryliuk
Copy link
Member

@bkyryliuk I agree it’s not much work to reimplement, though I do sense _at is more accurate than _on and thus adheres to other systems.

yep, agreed. curious to learn how hard would it be to do this rename in the FAB. It could be a good topic to discuss in slack.

@villebro
Copy link
Member

villebro commented Sep 3, 2020

I'm not familiar with _at vs _on, but if it is indeed established convention, I think it's a good change, but I'd prefer to see it happen via FAB. Wrt reimplementing vs using sqla-paranoid, I find reimplementing a better alternative in this case, as it doesn't seem to be a very actively maintained project.

@john-bodley
Copy link
Member Author

Closing per @bkyryliuk and @villebro's feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants