kolayne/helpline_telegraph

A Telegram helpline chat-bot. Enables pairs of people to have anonymous conversations

👋 Hey there! It's Nikolay Nechaev aka @kolayne

A Russian developer and a student at Innopolis University.

Social networks

Some statistics!

About me

Coming soon! one day.

helpline_telegraph's Issues

More sparing locking of `sent_invitations`

Currently, the whole sent_invitations table is locked during the whole process of sending out a group of invitations (either all invitations to a client or all invitations for an operator). For example,

helpline_telegraph/helpline_telegraph/core/invitations.py

Lines 118 to 121 in d791607

	cursor.execute("LOCK TABLE sent_invitations IN SHARE MODE")
	for client_chat_id in conversations_requesters:
	self._invite_operator_to_client(cursor, operator_chat_id, client_chat_id)

However, there is a little more sparing method.

We could introduce "pending invitations": invitations that have already been inserted into the database (for example, with invitation_message_id set to NULL) but haven't yet been actually sent. In this case, threads can first INSERT the invitations they are about to send, without explicit locking at all (PostgreSQL will automatically create row-level locks on the inserted rows, and the inserting transaction will acquire them, not lock the whole table), and after all the rows are inserted, the thread, which has made the insertions, actually sends out the invitations, updates the row and commits the transaction. Thus, other threads, trying to insert an invitation, which is being sent by another thread, will be paused just as they are now (because of the row-level locks) and will then fail to duplicate the invitation if the first transaction commits successfully. But unlike the current behavior, with the pending invitations the table doesn't get locked completely, that's why if there are invitations being sent for non-intersecting sets of users, transactions won't block each other

Invalid database scheme

The database schema is invalid because PostgreSQL does not support CHECK constraints that reference data that is not in the row being inserted or updated (docs). The current scheme does have these invalid CHECKs, but it seems like we actually only need them when creating a new conversation (i. e. INSERTing into conversations), so, I guess, we could ensure the data being inserted is valid in the INSERT query

DB scheme: `users.is_operator` field

A bool is_operator field should be in the users table instead of a field type of the enum type. It will simplify the schema by removing a user-defined type and allow operators to interact with the bot as if they were clients

The invitations sent before bot restart problem...

The current version of the bot has the following issue. All the invitation messages' ids are stored in the runtime variables instead of the database. That's why whenever the bot is restarted (for example, because of an update or because of an unhandled internal error) the invitation messages become undeletable. They aren't invalidated and operators still can join the conversations, but because the bot doesn't have the messages' ids, it is unable to delete them, so all the operators are likely to try to join the conversation.

I see two ways of solving this problem: either trying to handle the moment when the bot stops and removing the invitations at that moment (with warnings sent to users that something goes wrong and they'll have to request a conversation again), or moving the information about the invitation messages to the database. The second way, of course, provides a better user experience, however, it may be hard to implement, so the first solution can be used as a hotfix/workaround

Review the way of interaction with the database

I don't like the bot creates a new connection to the database each time it needs something from there. I guess, we should use a connection pool instead. Should search for tips from psycopg developers or look how others do this

Handle entities in messages

The ideal way is to copy all the entities from the original message to the copied one. It will be easy to implement this with the Telegram Bots API 5.0 (with the copyMessage method), however, currently, there is no telebot release, supporting this API version.

For now, users should at least warned, that their formatting is lost and messages are forwarded as a plain text

`invite_operators`: race condition

The piece of code
https://github.com/kolayne/anonymous_helpline_chatbot/blob/a2c20f78738bc1dff082d506a74714a7c60872b4/main.py#L44-L46
must be under the with conversation_starter_lock block:
https://github.com/kolayne/anonymous_helpline_chatbot/blob/a2c20f78738bc1dff082d506a74714a7c60872b4/main.py#L54-L72
because otherwise, the set of free operators could change between these code fragments, so the bot will send invitations to busy operators

Forbid sending messages to stopped conversations

If a conversation has finished, don't allow users to keep sending messages to each other

Don't ignore, which message do clients reply to

Forgot to implement this part!

https://github.com/kolayne/anonymous_helpline_chatbot/blob/d5dad8d150095f4a7def484325b8a7000734a863/main.py#L109-L110

Replace error-indicating return-values with exceptions

Exceptions should be raised instead to indicate errors instead of using error code integers

`logic.add_user` should have a docstring

Add doc for
https://github.com/kolayne/anonymous_helpline_chatbot/blob/2a287b49402da5496378385b5ce551f08ef743a5/logic.py#L7-L9

Make front-end independent from database

Move everything containing SQL queries to the core package

Don't duplicate information in `reflected_messages`, SQL

The simplest solution I've found is to insert new rows such that interlocutor1_chat_id is always less than interlocutor2_chat_id. This way we always know, what the order of the arguments in a query should be.
It's the second option from https://stackoverflow.com/a/15144061/11248508

Mention local client number in administrators' notifications

When a client reports feeling himself worse after a conversation with an operator, administrators receive the corresponding notification with the operator mentioned. The client number should be mentioned there too (however, administrators must not be able to determine who the client actually is)

Adding new operators (admin only)

Admins should have an interface for adding new operators

Typing indication

Indicate if the user I am chatting to is typing / has read my messages. Built-in Telegram typing event should be used to notify about typing

Media support

Add support for various kinds of messages, at least photos and videos, probably also voice messages and stickers

Handle messages' editions and deletions

When a user (either a client or an operator) edits a message, the reflected message should be updated too. Same for messages deletions (probably with a reservation that when a client deletes a message, it doesn't disappear for the operator, but is marked as deleted somehow)

Ability for operators to end conversations

Give operators ability to use the /end_conversation command

Document classes/functions/methods in the `core` package

Scheme docs + README

Add a readme, telling about usage and authors, add some comments to the scheme, explaining the unobvious meanings of tables/fields there

Smart operator selection

Do not select operators randomly. Perhaps decide which operators to choose based on:

• Who did the client chat with (and whether he/she liked the conversion)
• How much clients is an operator speaking to set the moment (after #18 each operator communicates with no more than 1 client at a time)
• Whether an operator is online and/or accepted the conversion

Callback query handler for valid scheme but unknown type

There is now a handler for an invalid callback (which will be triggered if the given callback data can't be json-decoded to a dictionary containing the 'type' key). However, I would also like to have the callbacks with present but unknown types to be handled. This should be a simple handler after all the others which should just tell the callback had an incorrect type

Don't repeat the same argument to an SQL query multiple times

I like the solution provided here

Improve invitations system

The invitations messages system doesn't currently work well when there are multiple clients and multiple operators in different states. For example, the following situations:

1. (not so bad) Assume there is a free operator and a couple of clients, which all request conversations. After an operator accepts one of the invitations, other invitations are preserved in the chat, which might confuse or distract the operator. All these invitations should probably be deleted when the operator accepts any one of them.
2. (significantly worse) Assume there are a bunch of busy operators, one free operator, and a client, who requests a conversation. Assume that the one free operator is not online, so he receives the invitation, but does not accept it. Some time later, the busy operators become free, but in the current version they don't receive the invitations from that client, they'll only receive invitations from new clients, if there are any. That's pretty bad, because nobody but that first free operator is able to join the conversation with the client.

Start/end report (admin only)

Report about start/end of conversation with operator designation to admin. Maybe should include rating of session (how?)
Rate may be:

• on a ten-point scale
• mood rate depending on the initial (worse/initial/better)

One session at time only

An operator shouldn't be able to talk with two users at the same time

Add tests for core race conditions / deadlocks

There are now many places for potential race conditions and deadlocks. It'd be nice to have tests for that part of the back-end

Deadlock because of inappropriate table locking method

SQL tables shouldn't be locked in the SHARE mode if they are to be modified, they are to be locked in the SHARE ROW EXCLUSIVE mode instead

DB scheme: use telegram id as a primary identifier

Use the telegram id as a primary identifier and use local id only when anonymity is required (when telling operators "User #N says the following"). Because all (or almost all) the requests sent to the database are using the telegram id, so an additional SELECT from the users table happens all the time

Add prefix for bot's "system" messages

It should be possible to distinguish between a message from bot and a message from a user you're in a conversation with