feat: Chat request implementation #14

Manually merged
awiteb merged 55 commits from awiteb/chat-request-and-response into master 2024-07-18 14:21:39 +02:00 AGit
Owner

Reslove #2

Chat request implementation of the OTMP protocol

Reslove https://git.4rs.nl/OxideTalis/oxidetalis/issues/2 Chat request implementation of the [OTMP](https://otmp.4rs.nl/) protocol
awiteb changed title from feat: Chat request implementation to WIP: feat: Chat request implementation 2024-07-09 02:21:34 +02:00
awiteb added the
Kind/Feature
label 2024-07-09 02:21:48 +02:00
awiteb added a new dependency 2024-07-09 02:25:45 +02:00
awiteb force-pushed awiteb/chat-request-and-response from 1356ae40d0 to 4abd0241e1 2024-07-09 12:17:28 +02:00 Compare
Author
Owner

I hate big PR, but there is no way to make it smaller. All thing are related and need each other.

I hate big PR, but there is no way to make it smaller. All thing are related and need each other.
awiteb force-pushed awiteb/chat-request-and-response from 870ffef0c1 to 33677cf055 2024-07-09 18:38:08 +02:00 Compare
awiteb force-pushed awiteb/chat-request-and-response from 33677cf055 to abed78fd77 2024-07-09 18:46:21 +02:00 Compare
awiteb force-pushed awiteb/chat-request-and-response from abed78fd77 to 6744537aab 2024-07-09 18:52:25 +02:00 Compare
awiteb force-pushed awiteb/chat-request-and-response from 14967dfd1c to 6f67474b5f 2024-07-12 22:24:41 +02:00 Compare
awiteb force-pushed awiteb/chat-request-and-response from 5a5ffb308f to 8392d87593 2024-07-13 22:22:19 +02:00 Compare
awiteb force-pushed awiteb/chat-request-and-response from 6e782d1e20 to 53db94c38f 2024-07-16 15:20:44 +02:00 Compare
awiteb changed title from WIP: feat: Chat request implementation to feat: Chat request implementation 2024-07-16 16:23:44 +02:00
Author
Owner

I'll re-base it on the master

I'll re-base it on the master
awiteb force-pushed awiteb/chat-request-and-response from 7512d1e25f to 1857365aac 2024-07-16 16:28:38 +02:00 Compare
Author
Owner

@Amjad50 Can you review it? If you can, take your time. If not, no problem, I will review it

@Amjad50 Can you review it? If you can, take your time. If not, no problem, I will review it
Member

I hate big PR, but there is no way to make it smaller. All thing are related and need each other.

haha, yah understandable.

I'll review it soon

> I hate big PR, but there is no way to make it smaller. All thing are related and need each other. haha, yah understandable. I'll review it soon
Amjad50 reviewed 2024-07-16 16:38:56 +02:00
Author
Owner

I hate big PR, but there is no way to make it smaller. All thing are related and need each other.

haha, yah understandable.

I'll review it soon

Take your time, there is couples TODO and FIXME I'll resolve it in another PR, after merging these

> > I hate big PR, but there is no way to make it smaller. All thing are related and need each other. > > haha, yah understandable. > > I'll review it soon Take your time, there is couples `TODO` and `FIXME` I'll resolve it in another PR, after merging these
Amjad50 requested changes 2024-07-17 03:19:53 +02:00
Dismissed
@ -0,0 +29,4 @@
/// Extension trait for the `DatabaseConnection` to work with the blacklist
/// table
pub trait BlackListExt {
/// Returns ture if the `blacklister` are blacklisted the
Member

ture -> true (use find and replace all, there are multiple of these)
blacklister has blacklisted

`ture` -> `true` (use find and replace all, there are multiple of these) `blacklister has blacklisted`
awiteb marked this conversation as resolved
@ -0,0 +62,4 @@
}
impl BlackListExt for DatabaseConnection {
#[logcall::logcall]
Member

I noticed that several methods have logcall but others don't, is it remaining from debugging session? or planning to keep it

I noticed that several methods have `logcall` but others don't, is it remaining from debugging session? or planning to keep it
Author
Owner

It was for debugging session, but I will use it on more functions

It was for debugging session, but I will use it on more functions
Author
Owner
image

There are a lot of functions that need to be checked, 300+ functions to check if the log is necessary or not, I hate this

<img width="360" alt="image" src="/attachments/985a139f-d8ae-4f7f-a782-f25891ac074e"> There are a lot of functions that need to be checked, 300+ functions to check if the log is necessary or not, I hate this
Author
Owner

Ok, it is just 100 functions

Ok, it is just 100 functions
awiteb marked this conversation as resolved
@ -0,0 +45,4 @@
.filter(InChatRequestsColumn::Sender.eq(sender.to_string()))
.one(self)
.await?
.is_none()
Member

since we are not dealing with the result. maybe a one statement approach to do

InChatRequestsEntity::insert(InChatRequestsActiveModel {
    recipient_id: Set(recipient.id),
    sender: Set(sender.to_string()),
    in_on: Set(Utc::now()),
    ..Default::default()
})
.on_conflict(
    OnConflict::columns([
        InChatRequestsColumn::RecipientId,
        InChatRequestsColumn::Sender,
    ])
    .update_column(InChatRequestsColumn::InOn)
    .to_owned(),
)
.exec(self)

here i'm updating InOn if needed, but can also be changed to do_nothing()

since we are not dealing with the result. maybe a one statement approach to do ```rust InChatRequestsEntity::insert(InChatRequestsActiveModel { recipient_id: Set(recipient.id), sender: Set(sender.to_string()), in_on: Set(Utc::now()), ..Default::default() }) .on_conflict( OnConflict::columns([ InChatRequestsColumn::RecipientId, InChatRequestsColumn::Sender, ]) .update_column(InChatRequestsColumn::InOn) .to_owned(), ) .exec(self) ``` here i'm updating `InOn` if needed, but can also be changed to `do_nothing()`
awiteb marked this conversation as resolved
@ -0,0 +30,4 @@
&self,
requester: &UserModel,
recipient: &PublicKey,
) -> ServerResult<Option<OutChatRequestsModel>>;
Member

should return true based on the name and description of the func

should return `true` based on the name and description of the func
Member

small nitpick (also applies to have_chat_request_to below)
I would name it get_chat_request_to, as have seems that it would be boolean

small nitpick (also applies to `have_chat_request_to` below) I would name it `get_chat_request_to`, as `have` seems that it would be boolean
awiteb marked this conversation as resolved
@ -0,0 +29,4 @@
/// Extension trait for the `DatabaseConnection` to work with the whitelist
/// table
pub trait WhiteListExt {
/// Returns ture if the `whitelister` are whitelisted the
Member

ture
The whitelister has whitelisted

ture The whitelister has whitelisted
awiteb marked this conversation as resolved
@ -113,0 +130,4 @@
}
async fn send(&self, conn_id: &Uuid, event: ServerEvent<Unsigned>) {
if let Some((_, user)) = self.read().await.iter().find(|(c, _)| *c == conn_id) {
Member

use get

if let Some(user) = self.read().await.get(conn_id) {
// ...
}
use `get` ```rust if let Some(user) = self.read().await.get(conn_id) { // ... } ```
awiteb marked this conversation as resolved
@ -51,0 +56,4 @@
/// New chat request from someone
ChatRequest { from: PublicKey },
/// New chat request response from someone
ChatRequestResponse { from: PublicKey, accepted: bool },
Member

in the client, you mentioned to make it alphabetical, should it be the same here?

in the client, you mentioned to make it alphabetical, should it be the same here?
Author
Owner

Yes, I forgot it

Yes, I forgot it
awiteb marked this conversation as resolved
@ -0,0 +50,4 @@
if try_ws!(db.is_blacklisted(&to_user, &from_public_key).await) {
return ServerEvent::message(
"You are unable to send a chat request because you are on the recipient's blacklist.",
Member

Maybe would be better to move this to WsError easier to manage errors
there are also others below this

Maybe would be better to move this to `WsError` easier to manage errors there are also others below this
Author
Owner

Right, this should be an error, not a message. I don't know how is this happened

Right, this should be an error, not a message. I don't know how is this happened
Author
Owner

I'll remove Message event it is actually useless. I don't know why I thought it was good.

I'll remove `Message` event it is actually useless. I don't know why I thought it was good.
Author
Owner

I don't know why I thought it was good.

Well, I remembered when I added it, it was a pain to add a new error (before ws_error macro) so I thought it was good for simple messages.

> I don't know why I thought it was good. Well, I remembered when I added it, it was a pain to add a new error (before `ws_error` macro) so I thought it was good for simple messages.
awiteb marked this conversation as resolved
@ -0,0 +18,4 @@
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
Member

I was thinking we can have one table for blacklist and whitelist.

since at any point in time a user can either be in blacklist or whitelist (or none), instead of doing

add_to_blacklist() {
    // ...
    remove_from_whitelist()?
    // ...
}

it can be a column containing the state, either whitelist or blacklist

I was thinking we can have one table for blacklist and whitelist. since at any point in time a user can either be in blacklist or whitelist (or none), instead of doing ```rust add_to_blacklist() { // ... remove_from_whitelist()? // ... } ``` it can be a column containing the state, either whitelist or blacklist
awiteb marked this conversation as resolved
@ -0,0 +41,4 @@
)
.col(ColumnDef::new(Blacklist::UserId).big_integer().not_null())
.col(ColumnDef::new(Blacklist::Target).string().not_null())
.col(ColumnDef::new(Blacklist::Reason).string_len(400))
Member

reason is unused

reason is unused
Author
Owner

I forgot to remove it, this should be a feature soon. Is good right?

I forgot to remove it, this should be a feature soon. Is good right?
Member

yah, if you want to add it, I think we can do in this PR, just change the whitelist table (or if we are merging both, it will be a special column for blacklist)

So we can either remove it or add it as a feature

yah, if you want to add it, I think we can do in this PR, just change the whitelist table (or if we are merging both, it will be a special column for blacklist) So we can either remove it or add it as a feature
Author
Owner

Let's make it later, since we'll merge them.
I don't want to think about it right now

Let's make it later, since we'll merge them. I don't want to think about it right now
awiteb marked this conversation as resolved
Amjad50 reviewed 2024-07-17 04:09:29 +02:00
@ -0,0 +41,4 @@
use crate::routes::{ApiError, ApiResult};
#[derive(Debug)]
pub struct Pagination {
Member

We can add EndpointArgRegister implementation and thus we can use it in the arg directly

async fn user_whitelist(
    req: &mut Request,
    depot: &mut Depot,
    pagination: Pagination,
) -> ApiResult<Json<Vec<WhiteListedUser>>>
impl EndpointArgRegister for Pagination {
    fn register(components: &mut OapiComponents, operation: &mut Operation, _arg: &str) {
        for parameter in Self::to_parameters(components) {
            operation.parameters.insert(parameter);
        }
    }
}

If we go with this, we can also use ToParameters macro to generate the implementation.
The issue with it is that it doesn't use ApiError, but if we are using it as a paremeter, salvo will handle the error reporting

We can add `EndpointArgRegister` implementation and thus we can use it in the arg directly ```rust async fn user_whitelist( req: &mut Request, depot: &mut Depot, pagination: Pagination, ) -> ApiResult<Json<Vec<WhiteListedUser>>> ``` ```rust impl EndpointArgRegister for Pagination { fn register(components: &mut OapiComponents, operation: &mut Operation, _arg: &str) { for parameter in Self::to_parameters(components) { operation.parameters.insert(parameter); } } } ``` If we go with this, we can also use `ToParameters` macro to generate the implementation. The issue with it is that it doesn't use `ApiError`, but if we are using it as a paremeter, `salvo` will handle the error reporting
Author
Owner

I'll see it, the reason why I don't put it as an argument, is because
Salvo ask me to implement ToSchema to it.

As I see, Salvo want it as one parameter, but the pagination
is two parameters. I'll see

I'll see it, the reason why I don't put it as an argument, is because Salvo ask me to implement `ToSchema` to it. As I see, Salvo want it as one parameter, but the pagination is two parameters. I'll see
Author
Owner

Ok, I get it. It ask me to implement ToSchema because of this

impl<T, const R: bool> EndpointArgRegister for QueryParam<T, R>
where
    T: ToSchema, // <----
{
    fn register(components: &mut Components, operation: &mut Operation, arg: &str) {
        let parameter = Parameter::new(arg)
            .parameter_in(ParameterIn::Query)
            .description(format!("Get parameter `{arg}` from request url query."))
            .schema(T::to_schema(components))
            .required(R);
        operation.parameters.insert(parameter);
    }
}

So I can just implement the EndpointArgRegister trait and not using QueryParam.

Thank you @Amjad50

Ok, I get it. It ask me to implement `ToSchema` because of this ```rust impl<T, const R: bool> EndpointArgRegister for QueryParam<T, R> where T: ToSchema, // <---- { fn register(components: &mut Components, operation: &mut Operation, arg: &str) { let parameter = Parameter::new(arg) .parameter_in(ParameterIn::Query) .description(format!("Get parameter `{arg}` from request url query.")) .schema(T::to_schema(components)) .required(R); operation.parameters.insert(parameter); } } ``` So I can just implement the `EndpointArgRegister` trait and not using `QueryParam`. Thank you @Amjad50
awiteb marked this conversation as resolved
awiteb added 4 commits 2024-07-17 18:15:31 +02:00
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
feat: Use TableCreateStatement::foreign_key for foreign keys
All checks were successful
DCO checker / DCO checker (pull_request) Successful in 16s
Rust CI / Rust CI (pull_request) Successful in 5m3s
7ef02d05d0
Signed-off-by: Awiteb <a@4rs.nl>
awiteb added 2 commits 2024-07-17 18:36:30 +02:00
Signed-off-by: Awiteb <a@4rs.nl>
chore: Use HashMap::get instead of Iter::find
Some checks failed
DCO checker / DCO checker (pull_request) Successful in 3s
Rust CI / Rust CI (pull_request) Has been cancelled
48acab2519
Signed-off-by: Awiteb <a@4rs.nl>
awiteb added 1 commit 2024-07-17 18:38:18 +02:00
fix: Sort ServerEventType::ChatRequestResponse event alphabetically
All checks were successful
DCO checker / DCO checker (pull_request) Successful in 7s
Rust CI / Rust CI (pull_request) Successful in 4m43s
c13d0fc59b
Signed-off-by: Awiteb <a@4rs.nl>
awiteb added the
Status
Waiting On Author
label 2024-07-17 22:42:58 +02:00
awiteb added 9 commits 2024-07-17 22:43:55 +02:00
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Awiteb <a@4rs.nl>
feat: Create unique index for UserId and Target
All checks were successful
DCO checker / DCO checker (pull_request) Successful in 4s
Rust CI / Rust CI (pull_request) Successful in 4m32s
868ec66b68
Signed-off-by: Awiteb <a@4rs.nl>
Amjad50 reviewed 2024-07-18 10:01:56 +02:00
@ -0,0 +103,4 @@
whitelister: &UserModel,
target_public_key: &PublicKey,
) -> ServerResult<bool> {
whitelister
Member

you can use get_user_status here and also is_blacklisted. also in remove... functions

you can use `get_user_status` here and also `is_blacklisted`. also in `remove...` functions
awiteb marked this conversation as resolved
awiteb added 1 commit 2024-07-18 10:07:59 +02:00
Signed-off-by: Awiteb <a@4rs.nl>
awiteb force-pushed awiteb/chat-request-and-response from 11b64caa85 to 21880c5f82 2024-07-18 10:17:50 +02:00 Compare
awiteb added 1 commit 2024-07-18 10:33:34 +02:00
change: Rename OutChatRequestsExt::have_chat_request_to function to get_chat_request_to
All checks were successful
DCO checker / DCO checker (pull_request) Successful in 3s
Rust CI / Rust CI (pull_request) Successful in 4m11s
39e7861917
Signed-off-by: Awiteb <a@4rs.nl>
awiteb added 1 commit 2024-07-18 11:29:02 +02:00
chore: More logging
All checks were successful
DCO checker / DCO checker (pull_request) Successful in 4s
Rust CI / Rust CI (pull_request) Successful in 4m28s
6f0b88afb1
Signed-off-by: Awiteb <a@4rs.nl>
awiteb requested review from Amjad50 2024-07-18 11:29:13 +02:00
awiteb added
Status
Waiting On Review
and removed
Status
Waiting On Author
labels 2024-07-18 11:29:23 +02:00
Amjad50 approved these changes 2024-07-18 13:16:23 +02:00
Amjad50 left a comment
Member

Nice

Nice
awiteb manually merged commit 82132fc446 into master 2024-07-18 14:21:39 +02:00
awiteb referenced this pull request from a commit 2024-07-22 23:32:01 +02:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
Reference: OxideTalis/oxidetalis#14
No description provided.