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

feat: implement v2 for get /countries/leaderboard #97

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

dagmawig
Copy link
Contributor

Fixes issue #96

Added leaderboard method to V2 API. Test automatically clears raw_capture_feature table then populates it with known data, completes the test and clears the table at the end of testing.

image

@dadiorchen dadiorchen requested a review from kparikh9 March 22, 2022 02:40
Copy link
Contributor

@kparikh9 kparikh9 left a comment

Choose a reason for hiding this comment

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

Code is fine, just left some comments that you can consider addressing

),
);
expect(response.status).toBe(200);
expect(response.body.countries[0]).toMatchObject({
Copy link
Contributor

Choose a reason for hiding this comment

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

For the leaderboard test, it might be beneficial to check which specific country is first in the array. I believe tree 1 is located in USA, tree 2 is located in Canada, so you could expect either Canada or USA. Maybe include more trees in your seed file to definitively have 1 country appear in the first array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added another tree planted in the US to make the top country specific. Now it checks if the country is US and if the number of trees planted is 2.

async function seed() {
knexConfig.searchPath = [process.env.DATABASE_SCHEMA, 'webmap'];
const serverCon = knex(knexConfig);
const response = await serverCon.transaction(async (trx) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to rename the existing raw_capture_feature table to another name (say temp_raw_capture_feature), then fill a newly created table with your seed data and name it to raw_capture_feature for your test. The test can proceed to completion, then you can delete the created table and rename temp_raw_capture_feature back to raw_capture_feature. This way will avoid clearing actual test data once ETL scripts begin to populate raw_capture_feature, and you won't have to rewrite leaderboard tests to account for the the new data in the table. You can use this knex documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes, I know this is annoying, but, just don't touch the tables in public , you all can freely delete data under webmap schema, that's totally fine, because we didn't use that table for web map presentation.

And the next step would be use dedicated database for test, so we can do anything safely, there already are good practice in other our microservice for this, so we can learn what they are doing

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, I run a docker image of postgresql, and run db-migrate to set up the database, then I can run the test fastly, for other API service, like: earnings-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not deleting any data from the public schema. The data addition and deletion is all in the webmap schema. So I don't see the need to create new tables because the intention is to finally add a single set of data that would be used to do all the tests on the webmap schema.

@dadiorchen
Copy link
Contributor

@dagmawig thank your for you great job!

@dadiorchen
Copy link
Contributor

@dagmawig Oh, I have reviewed this PR is good enough to merge, I just forgot to click the button.

@dadiorchen dadiorchen merged commit c7d32b9 into Greenstand:main Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

🎉 This PR is included in version 1.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants