-
-
Notifications
You must be signed in to change notification settings - Fork 18
[16.0][IMP] ai_oca_bridge #21
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
base: 16.0
Are you sure you want to change the base?
Conversation
72356a5 to
4732946
Compare
4732946 to
29dfab1
Compare
| try: | ||
| response = {"id": self._get_channel().message_post(**response).id} | ||
| except Exception as e: | ||
| response = {"error": "mensaje de error para la IA"} |
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 shouldn't be here....
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.
Thanks for the review.
I've already fixed the mistake.
29dfab1 to
2d0cd36
Compare
etobella
left a comment
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.
user_id was intended for doing this. You shouldn't replace it for a new field.
Not changing it is part of a wrong configuration. We can talk if we should improve it somehow, but we don't need this extra field for sure.
Moreover, you are adding using the payload. it is not effective. You should use create_uid for that. In any case, from a Legal perspective, is important to notice that the message comes from an AI system, so for me we shouldn't do this
You're right saying that user_id shouldn't change its purpose. What I would suggest then is that it should be displayed in the view so that the user can be selected, because right now it is defaulted, see here I mention this because currently, because with current behavior, if we wanted to link the bridge to an agent, we would have to log in with the agent's user and create the bridge, so that the bridge uses agent's user. On the other hand, if we just show the user_id, it would solve the problem of the user the message being posted by the wrong user, and also the issue of permissions, as we control the user permissions. This would remove a big chunk of the extra logic I've added before. |
|
Why? Just change user id in the view. Isn't it working? |
e66a399 to
6481c6d
Compare
6481c6d to
accd0d1
Compare
|
@etobella I believe that this solves the issue of permissions and allows you to choose the user who will manage the bridge. |
|
|
||
| def _process_response_message(self, response): | ||
| return {"id": self._get_channel().message_post(**response).id} | ||
| return {"id": self._get_channel().sudo().message_post(**response).id} |
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.
Don't use a sudo here. If the user has no permissions, it should fail IMO
This PR solves #20
While working on the permissions issue, I noticed that when posting messages, the user who created the bridge was always listed as the author. In certain scenarios, this behaviour was illogical.
To fix this, I added two fields to “ai.bridge”:
user_execution_behavior: A selection field with two options: link the bridge to the user with whom it will always be executed, or execute it with the user who activates it.user_predefined: Stores the default user when the first option is chosen.All this meant that I had to handle the logic in the
_process_responseand_process_response_messagemethods.@BinhexTeam