-
-
Notifications
You must be signed in to change notification settings - Fork 24
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 User Status Log #372
base: main
Are you sure you want to change the base?
Add User Status Log #372
Conversation
56e4deb
to
fe5896b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what this is going for, but I think there's a better design here. I'd like to see this model tied to a specific use case -- maybe a banning feature, say -- so that we can get a better feel for how and where this model would be used. For example, perhaps one simple schema could be:
class ModeratorLog:
id
moderator_id
user_id(nullable=true)
description
timestamp
which would be generic to all moderator actions, including automatic actions like verifying a user.
Also, let's clean up the role setters (set_is_banned
) as discussed earlier
change_description = Column(String, nullable=False) | ||
|
||
#: When should this status change expire/revert, defaults to never. | ||
expiry = Column(DateTime, default=None, nullable=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the semantics here are all in change_description
which is unstructured text, there's nothing actionable we can do even if we know expiry
. This seems like something we could fold into change_description
itself
timestamp = Column(DateTime, default=datetime.utcnow, nullable=False) | ||
|
||
#: Describes the status change that occurred. | ||
change_description = Column(String, nullable=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description
since the change_
is implicit by dint of this being a log
#: When should this status change expire/revert, defaults to never. | ||
expiry = Column(DateTime, default=None, nullable=True) | ||
|
||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait on these until we know how to use them
@@ -81,6 +81,40 @@ def __repr__(self): | |||
return f"<Role({self.id}, {self.name!r})>" | |||
|
|||
|
|||
class UserStatusLog(AmbudaUserMixin, Base): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove UserMixin
here as this is not a user
#: Primary key. | ||
id = pk() | ||
|
||
#: The user whose status was changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state changes should also indicate the user who made the change (or perhaps ambuda-bot
if there isn't one)
Diff 2/3 for #224