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

Fix: Include quest completion timestamp in get_quest_participants response #359

Open
wants to merge 3 commits into
base: testnet
Choose a base branch
from

Conversation

TropicalDog17
Copy link

@TropicalDog17 TropicalDog17 commented Dec 25, 2024

Close #358

@TropicalDog17 TropicalDog17 changed the title Fix: Include quest completion timestamp in get_quest_participants res… Fix: Include quest completion timestamp in get_quest_participants response Dec 25, 2024
@Marchand-Nicolas
Copy link
Collaborator

Hello @TropicalDog17 when will the PR be ready for review ?

@TropicalDog17 TropicalDog17 marked this pull request as ready for review January 3, 2025 03:01
@TropicalDog17
Copy link
Author

Hey please review this PR. Thank you so much! @Marchand-Nicolas

@@ -229,8 +229,10 @@ pub fn load() -> Config {
let args: Vec<String> = env::args().collect();
let config_path = if args.len() <= 1 {
"config.toml"
} else {
} else if (args.len() > 1) && (args.get(1).unwrap().contains("toml")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why ?

Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

As said in the issue, the route we want to update is src/endpoints/admin/quest/get_quest_participants.rs, not src/endpoints/get_quest_participants.rs,

@@ -26,4 +26,8 @@ pub mod tests {
let response = client.get(endpoint).send().await.unwrap();
assert_eq!(response.status(), StatusCode::OK);
}

// #[tokio::test]
// pub async fn test_get_quest_participants() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this

return (StatusCode::OK, Json(res)).into_response();
(StatusCode::OK, Json(res)).into_response()
}
#[cfg(test)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have the tests, but it was a good idea to make sure everything was working. The problem is that if we run tests in production, it'll reset the db

@Marchand-Nicolas Marchand-Nicolas added the ❌ Change request Change requested from reviewer label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❌ Change request Change requested from reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update get_quest_participants to return the timestamp of quest completion
2 participants