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

[FEATURE] Feedback system #31

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[FEATURE] Feedback system #31

wants to merge 13 commits into from

Conversation

SorkoPiko
Copy link

@SorkoPiko SorkoPiko commented Aug 24, 2024

Feedback system

New Endpoints:

GET /v1/mods/{id}/versions/{version}/feedback:

Get mod feedback - only accessible to mod dev or admins

Example Response
{
  "score": 
  	{
  		"score" : 1,
  		"positive": 2,
  		"negative": 1
  	},
  "mod_id": "sorkopiko.dailystreak",
  "mod_version": "1.1.0",
  "feedback": [
  	{
  		"reviewer": 
  			{
  				"id": 129,
  				"display_name": "SorkoPiko"
  				"admin": false,
  				"dev": true
  			},
  		"feedback_type": "Positive",
  		"feedback": "Really cool mod! Might wanna fix a few visual bugs though, but overall amazing.",
  		"decision": false
  	}
  ]
}

POST /v1/mods/{id}/versions/{version}/feedback:

Add/update mod feedback - only accessible to verified devs and admins (mod devs can only leave notes)

Request Body Example
{
  "feedback_type": "Positive",
  "feedback": "Really cool mod! Might wanna fix a few visual bugs though, but overall amazing."
}

DELETE /v1/mods/{id}/versions/{version}/feedback:

Delete feedback.

Request Body Example
{
  "id": 4
}

New Tables:

  • mod_feedback

Columns:

  • mod_version_id: Mod version ID, NOT mod version string
  • reviewer_id: Developer ID of reviewer
  • feedback: Feedback string
  • decision: Whether this feedback decided the status of the mod
  • type: Positive, Negative, Suggestion or Note

Notes:

  • Updated API spec with new endpoints
  • Feedback type in requests is case-sensitive
  • Mod devs can only leave notes

@SorkoPiko
Copy link
Author

Just to clarify - this is feedback for PENDING mods, to be given by verified devs/admins. This isn't for public use.

@SorkoPiko
Copy link
Author

@qimiko I've updated the comment with the new changes using the osu-like system you requested.
Thanks!

@Fleeym
Copy link
Collaborator

Fleeym commented Aug 27, 2024

I'll take a look at this today when I'm back from work, looks good at first glance

@SorkoPiko
Copy link
Author

I'll take a look at this today when I'm back from work, looks good at first glance

alright thanks. @qimiko had already given me some improvements which i implemeted this morning. appreciate it!

Copy link
Collaborator

@Fleeym Fleeym left a comment

Choose a reason for hiding this comment

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

Looks good overall, just some small nitpicks I personally had. We should also think about implementing something clientside for this

migrations/20240826202953_feedback_v2.up.sql Outdated Show resolved Hide resolved
src/endpoints/mod_feedback.rs Outdated Show resolved Hide resolved
src/endpoints/mod_feedback.rs Outdated Show resolved Hide resolved
src/endpoints/mod_feedback.rs Outdated Show resolved Hide resolved
@SorkoPiko
Copy link
Author

Alright, I applied the first 3 changes. I'm still not sure why "latest" bugs, but I'll look into it now.

@SorkoPiko
Copy link
Author

Latest is now fixed. Let me know if you need anything else!

@SorkoPiko
Copy link
Author

I've just added a delete endpoint.

@SorkoPiko SorkoPiko requested a review from Fleeym August 28, 2024 20:11
@qimiko
Copy link
Contributor

qimiko commented Sep 2, 2024

ok so i made like a million reviews i'm sorry 😭 but i left my thoughts on the current prq. you're free to question anything i say

Copy link
Contributor

@qimiko qimiko left a comment

Choose a reason for hiding this comment

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

very close 🙂 these specifically are my thoughts on what was added

feedback TEXT COLLATE pg_catalog."default" NOT NULL,
decision BOOLEAN NOT NULL DEFAULT false,
type feedback_type NOT NULL,
dev bool NOT NULL DEFAULT false,
Copy link
Contributor

Choose a reason for hiding this comment

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

this could get a little confusing, considering that "this person is a developer of this mod" is not constant. i'd rather this check be done during the sql query (like the admin field) instead of stored in the table

Copy link
Author

Choose a reason for hiding this comment

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

ah alr

}

#[delete("/v1/mods/{id}/versions/{version}/feedback")]
pub async fn delete_mod_feedback(
data: web::Data<AppData>,
path: web::Path<GetModFeedbackPath>,
payload: web::Json<DeleteModFeedbackPayload>,
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of payload, put the id as part of the path:
/v1/mods/{id}/versions/{version}/feedback/{feedback_id}

this is more consistent with the rest of the api and also consistent with what a delete request is

Copy link
Author

Choose a reason for hiding this comment

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

yeah ok

@@ -112,34 +109,31 @@ pub async fn post_mod_feedback(
.await
.or(Err(ApiError::TransactionError))?;

Ok(HttpResponse::NoContent())
Ok(web::Json(ApiResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this behavior is somewhat inconsistent considering the rest of the post endpoints don't do this, but i can see why you did it. i'll probably think about it further

Copy link
Author

Choose a reason for hiding this comment

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

ok, i'll leave it as it is rn then

Copy link
Contributor

Choose a reason for hiding this comment

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

on second thought, nocontent is probably the generally the best move

Copy link
Author

Choose a reason for hiding this comment

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

how would you get the id? through the GET endpoint?

Copy link
Contributor

Choose a reason for hiding this comment

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

ye, i think if the get feedback returns the author's feedback as well, then it makes it easy to associate feedback with the id on the client (so they can delete)

i wouldn't assume people are storing the id locally anyways

Copy link
Author

Choose a reason for hiding this comment

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

ok

WHERE mf.mod_version_id = "#
);
query_builder.push_bind(version.id);
if dev_only {
Copy link
Contributor

Choose a reason for hiding this comment

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

i was thinking about this a little more and i think it'd be better to allow returning the current user's feedback as well, like instead of dev_only you have filter_user: Option<i32> where None shows all feedback and having some value returns the feedback by that user id + the dev

so it doesn't just seem like your feedback was thrown into the void

Copy link
Author

Choose a reason for hiding this comment

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

alr

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

Successfully merging this pull request may close these issues.

3 participants