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 User Status Log #372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

AkshayPall
Copy link
Contributor

@AkshayPall AkshayPall commented Sep 25, 2022

Diff 2/3 for #224

Copy link
Contributor

@akprasad akprasad left a 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)
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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):
Copy link
Contributor

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.
Copy link
Contributor

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)

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.

2 participants