Feed system, user/organization following [MOD-535]#739
Conversation
…ir redirects, and client authorizations
| #[sqlx(type_name = "text")] | ||
| #[sqlx(rename_all = "snake_case")] | ||
| pub enum EventType { | ||
| ProjectCreated, |
There was a problem hiding this comment.
version created/ project updated event is missing
There was a problem hiding this comment.
I wasn't under the impression we wanted those implemented in this first pass and assumed we would follow up this PR with ones implementing those.
Would you like me to implement those in this PR?
There was a problem hiding this comment.
Yeah I think it would be worth implementing
| let organization_id = project_create_data.organization_id.map(|id| id.into()); | ||
| insert_project_create_event(project_id, organization_id, ¤t_user, transaction) | ||
| .await?; |
There was a problem hiding this comment.
the project creation event should not happen here. Users should only be sent notifications if a project has been approved / has a public status. You can put this in the code where we send discord webhooks to the public channel (in project edit)
There was a problem hiding this comment.
I think this location makes the most sense for the feeds system. I agree if it were immediately sending notifications to users, but it's not. This is just registering the event when the thing happens.
When we are getting the user's feed, we filter the projects returned to them only to the ones they're authorized to see. This means (unless I'm wrong about the filter_authorized_projects method) that they won't see the project until it's public, unless they're for some reason authorized to see it (a mod, on the same team I'm guessing, etc). I think the events should reflect the truth as well as they can, then we interpret those events how we want when we're generating a user's feed.
| } | ||
| } | ||
|
|
||
| pub async fn follow<T, TId, Fut1, Fut2>( |
There was a problem hiding this comment.
this pattern also makes the code very hard to follow, I would just use the same pattern project following has, with 2 separate routes for both orgs and users, and put the code there. I understand it removes some repeated code, but it's not worth the cost esp bc nothing else will have following, this seems like you're designing a system with the intention we'll have 10+ things support following
There was a problem hiding this comment.
Yeah, this turned out much clunkier than I wanted. I disagree with the implication that something needs 10+ usages to be worth avoiding duplication, but in this case, I think the benefit is slim enough that I'll inline and duplicate them as you'd like.
This could have been done much more elegantly with traits, but unfortunately async fn isn't yet valid in trait definitions, so this was the best I could come up with. That should be coming with Rust 1.75 in December, too bad we don't have it now.
There was a problem hiding this comment.
I was exaggerating yeah but I think it would be worth inlining these
There was a problem hiding this comment.
see my initial comment on feeds, will rev this file later since I imagine it might change later
# Conflicts: # src/database/models/ids.rs # src/models/ids.rs # src/routes/v3/mod.rs # tests/common/api_v3/mod.rs # tests/common/asserts.rs # tests/common/dummy_data.rs
|
|
||
| // On publish event, we send out notification using team *owner* as publishing user, at the time of mod approval | ||
| // (even though 'user' is the mod and doing the publishing) | ||
| let owner_id = | ||
| TeamMember::get_owner_id(project_item.inner.team_id, &mut *transaction) | ||
| .await?; | ||
| if let Some(owner_id) = owner_id { | ||
| insert_project_publish_event( | ||
| id.into(), | ||
| project_item.inner.organization_id, | ||
| owner_id, | ||
| &mut transaction, | ||
| ) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
hmm, I think this should use the org ID if the project belongs to the org, and if not it should use the owner ID. could it be changed to that?
4 participants
No description provided.