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 6 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
@Marchand-Nicolas
Copy link
Collaborator

Hello @TropicalDog17 everything good ?

@TropicalDog17
Copy link
Author

Hi sorry for the delay, can you review @Marchand-Nicolas

args.get(1).unwrap()
} else {
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 these changes, you can revert

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

// #[tokio::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 these comments

@@ -99,3 +101,168 @@ pub async fn get_quest_participants_handler(
let participants_json = json!({ "participants": participants });
(StatusCode::OK, Json(participants_json)).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 these comments

@@ -58,11 +58,13 @@ pub async fn get_quest_participants_handler(
doc! { "$match": { "task_id": { "$in": &task_ids } } },
doc! { "$group": {
"_id": "$address",
"task_ids": { "$addToSet": "$task_id" }
"task_ids": { "$addToSet": "$task_id" },
"max_timestamp": { "$max": "$timestamp.$numberLong" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error aggregating completed tasks: Kind: Command failed: Error code 16410 (Location16410): FieldPath field names may not start with '$'. Consider using $getField or $setField., labels: {}

@Marchand-Nicolas
Copy link
Collaborator

Hello @TropicalDog17 everything good ?

@TropicalDog17
Copy link
Author

Hi @Marchand-Nicolas I struggle to debug this, since I am not quite able to setup the enviroment properly, can you help me? Sorry for any inconvenience

@Marchand-Nicolas
Copy link
Collaborator

Hello @TropicalDog17 here is a guide to help you setup the project: https://github.com/lfglabs-dev/api.starknet.quest/blob/testnet/README.md
You need to update this file: src/endpoints/admin/quest/get_quest_participants.rs
An example pipeline has been provided in the issue. You might just need to copy paste it

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