-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
User refactor #50
base: master
Are you sure you want to change the base?
User refactor #50
Conversation
sorry for couple of last minute fixes |
Sorry that I took a while to respond, I was on Easter break :) I'll check your PR right now 😄 |
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.
So many things to check! 😮
Thanks for everything, but there are a few things that need to be changed before I can merge this...
@@ -31,9 +35,10 @@ class User(TableDeclarativeBase): | |||
first_name = Column(String, nullable=False) | |||
last_name = Column(String) | |||
username = Column(String) | |||
last_seen = Column(DateTime, default=datetime.datetime.utcnow) |
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.
Changing existing tables... is troublesome, as all current users of the bot would need to migrate somehow their database to the new schema.
Either we provide a migration script, or we put the new info in a new, different table instead... 🤔
I could probably help with that, but it may take a while...
@@ -200,7 +208,7 @@ def __wait_for_regex(self, regex: str, cancellable: bool = False) -> Union[str, | |||
if match is None: | |||
continue | |||
# Return the first capture group | |||
return match.group(1) | |||
return match.group(1) or match.group(2) |
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.
This change is unnecessary and possibly dangerous 🤔 , please revert it!
In your use case, you should be able to structure your regex differently to have a single capture group.
Something like... (?:user_|\@)(.*)
should work.
elif callback.data == "toggle_receive_orders": | ||
admin.receive_orders = not admin.receive_orders | ||
elif callback.data == "toggle_create_transactions": | ||
admin.create_transactions = not admin.create_transactions | ||
elif callback.data == "toggle_display_on_help": | ||
admin.display_on_help = not admin.display_on_help | ||
elif callback.data == "cmd_remove": | ||
self.session.query(db.Admin).filter(db.Admin.user_id==user.user_id).delete() | ||
message = self.bot.send_message(self.chat.id, strings.conversation_confirm_admin_dismissal) |
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.
What happens when an user removes themselves from Manager? Does the bot stay without any managers?
What if an user removes another one from managers, but the other still has a conversation open?
This merge has two groups of changes
Admin part:
last_seen
attribute to track existing users activityImage upload part