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 config limits for portal rooms #469

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Half-Shot
Copy link
Contributor

Trying to solve #466

I'm not sure if this works, but would value a review to see how close I am.

Copy link
Member

@tulir tulir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should work for preventing portal creation, but the errors are probably not displayed in any sensible way currently.

mautrix_telegram/portal/base.py Outdated Show resolved Hide resolved
@Half-Shot
Copy link
Contributor Author

This works for portals, but we'd also like to limit for puppeted users. Looking at that now...

Copy link

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DBPortal.count() for some reason does not exceed 1.

Comment on lines 58 to 63
rows = cls.db.execute(select([func.count()]))
try:
count, = next(rows)
return count
except StopIteration:
return 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baby's first Python commits (don't commit blindly):

Suggested change
rows = cls.db.execute(select([func.count()]))
try:
count, = next(rows)
return count
except StopIteration:
return 0
return cls.db.query(func.count('*')).scalar()

Please test this. I haven't actually run this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good unfortunately,

AttributeError: 'Engine' object has no attribute 'query'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table to count rows in probably needs to be specified somewhere 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh derp, I thought that was implied somewhere

@@ -64,3 +69,7 @@ def get_by_username(cls, username: str) -> Optional['Portal']:
@classmethod
def all(cls) -> Iterable['Portal']:
yield from cls._select_all()

@classmethod
def get_by_mxid(cls, mxid: RoomID) -> Integer:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to have been changed accidentally

@@ -53,6 +53,11 @@ def get_by_tgid(cls, tgid: TelegramID, tg_receiver: TelegramID) -> Optional['Por
def find_private_chats(cls, tg_receiver: TelegramID) -> Iterable['Portal']:
yield from cls._select_all(cls.c.tg_receiver == tg_receiver, cls.c.peer_type == "user")

@classmethod
def count(cls) -> int:
count = cls.db.execute(select([func.count('*')]).select_from(Table("portal", MetaData()))).scalar()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might work

Suggested change
count = cls.db.execute(select([func.count('*')]).select_from(Table("portal", MetaData()))).scalar()
count = cls.db.execute(cls.t.select([func.count('*')])).scalar()

Copy link

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests showed that the room limit is enforced now. 👍

@Half-Shot
Copy link
Contributor Author

This needs to count the number of rooms where mxid is not null

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

Successfully merging this pull request may close these issues.

4 participants