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

Iss1019feat #1030

Merged
merged 21 commits into from
Feb 19, 2022
Merged

Iss1019feat #1030

merged 21 commits into from
Feb 19, 2022

Conversation

yosoycentli
Copy link
Contributor

This Pull Request add the next feature related with the 1019 issue (#1019):

You can see data from 30 European countries related to the different COVID-19 variants, as you can see in the pictures.

Source of the data: https://www.ecdc.europa.eu/en/publications-data/download-todays-data-geographic-distribution-covid-19-cases-worldwide

There are two new endpoints:

  • /v3/covid-19/variants/countries/

Screen Shot 2022-01-14 at 16 40 32

  • /v3/covid-19/variants/countries/{country}

Screen Shot 2022-01-14 at 16 40 19

I'm still working on the test for this scraper, the swagger documentation and I need to remove the instance "Redis commander" that I install in order to see what I load to the Redis database http://joeferner.github.io/redis-commander/.

I'm sure that there will be comments and modifications requests from the maintainers but I think is a good start what I've done here.

Best Regards Centli Garcés

Copy link
Collaborator

@ebwinters ebwinters left a comment

Choose a reason for hiding this comment

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

The code looks pretty good, similar to the rest of the code base. Left a few comments, when you are ready to publish (and have docs+tests) I will review again. Thanks for the work here, I'm sure it will be picked up and used by people around the world!


router.get('/v3/covid-19/variants/countries/:country?', async (req, res) => {
const { allowNull } = req.query;
const { country: countryName, yearWeek, variant } = req.params;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the yearWeek and variant parameters being used

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 put them there since I wanted the API had to have these parameters, but I think it's better to remove them and later when they're created added again.

Comment on lines 28 to 41
// *to here*

// Take this code below as an example:
// router.get('/v3/covid-19/countries/:query', async (req, res) => {
// const { yesterday, twoDaysAgo, strict, allowNull } = req.query;
// const { query } = req.params;
// let countries = JSON.parse(await redis.get(wordToBoolean(yesterday) ? keys.yesterday_countries : wordToBoolean(twoDaysAgo) ? keys.twoDaysAgo_countries : keys.countries))
// .filter(country => country.country.toLowerCase() !== 'world').map(fixApostrophe);
// countries = splitQuery(query)
// .map(country => nameUtils.getWorldometersData(countries, country, strict !== 'false'))
// .filter(value => value).map(country => !wordToBoolean(allowNull) ? nameUtils.transformNull(country) : country);
// if (countries.length > 0) res.send(countries.length === 1 ? countries[0] : countries);
// else res.status(404).send({ message: 'Country not found or doesn\'t have any cases' });
// });
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove once ready to publish draft

const csvUtils = require("../../utils/csvUtils");

const PATH =
"https://opendata.ecdc.europa.eu/covid19/virusvariant/csv/data.csv";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep this in mind when writing the Swagger docs
image

return obj;
}, {});

console.log(dataByCountry);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove when publishing draft, or alternatively put a log statement with the length of the result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll remove this "console.log()"

Thanks for checking this.

@@ -71,6 +71,34 @@ describe.skip('TESTING /v3/covid-19/gov/canada', () => {
});
});

// here
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put into own testing file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this new variants endpoint needs its own test file, I'm working on that.

@yosoycentli yosoycentli marked this pull request as ready for review January 18, 2022 00:22
@yosoycentli
Copy link
Contributor Author

Hi @ebwinters,
I think I cover all your comments about the code, I hope you can check the changes, especially the swagger part.

Best regards from Mexico City.

"variantsECDC": {
"type": "array",
"items": {
"type": "string"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't look right. See how other endpoints do this where they return an array of type x and then go on to declare x as the object structure of the element. Like

"updated": dateTime,
"country": string,
"yearWeek": string...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right, sorry about this.
I'm going to do fix it.

@yosoycentli
Copy link
Contributor Author

@ebwinters
I think it's ok now :)

Thank you very much for your effort, time and comments, they're very helpful.

ebwinters
ebwinters previously approved these changes Jan 19, 2022
@ebwinters
Copy link
Collaborator

It's approved. Unit tests will fail for now until #1031 is merged. Once that is in, pull master, and the tests should all pass.

Also linting failed, check out why in the workflow run and fix that please

@yosoycentli
Copy link
Contributor Author

Hi @ebwinters,
I fixed these eslint problems and I think it's finally everything ok.

Thank you so much for your patience with me :)

@ebwinters
Copy link
Collaborator

still seeing linting issues (unrelated to your PR but still fix please)
image

@yosoycentli
Copy link
Contributor Author

All right then, I'll fix it right now :)

…ix the linting problem, as there are variables assigned that are never used.
@yosoycentli
Copy link
Contributor Author

I commented the lines 2-5 to fix the linting problem. At least in my computer that is enough to pass the eslint check.
:)

@ebwinters
Copy link
Collaborator

now your unit tests are failing

@yosoycentli
Copy link
Contributor Author

I'll check these, I truly don't know why this is happening.

Thanks for your patience.

@ebwinters
Copy link
Collaborator

@yosoycentli it's still failing, just checking back in

@yosoycentli
Copy link
Contributor Author

I'm working on these tests, I still don't know why it's happening but I'll find out, I'm sure of that.
:)

…test' and the 'test-single' in the package.json file in order to get time enough to complete all tests
@yosoycentli
Copy link
Contributor Author

Hi @ebwinters

I want to know if the limit of 200000 in the --timeout from the test and test-single scripts is something that I can change because I modified this number to 300000 and now all my tests are passing correctly.
:)

@ebwinters
Copy link
Collaborator

Hi @ebwinters

I want to know if the limit of 200000 in the --timeout from the test and test-single scripts is something that I can change because I modified this number to 300000 and now all my tests are passing correctly. :)

yea totally fine that's not a hard limitation

@yosoycentli
Copy link
Contributor Author

Hi, @ebwinters,

I finally understood why my tests don't pass, the reason is that the page where the scraper gets the information is usually under maintenance, so it's common that the tests don't pass.

I was wondering what is recommended to do in these cases, I noticed that there are tests that are pending, should I put my tests as pending?

Without anything else for the moment again I appreciate your patience with me.

Greetings from Mexico City

@ebwinters
Copy link
Collaborator

no it's fine if they are pending

@yosoycentli
Copy link
Contributor Author

All right then @ebwinters ;).

@yosoycentli
Copy link
Contributor Author

Hi @ebwinters ,
I realized that the tests finally passed so I would like to know if this is enough to merge the code or if it's better to put my tests as "pending".

@ebwinters ebwinters merged commit 09b5646 into disease-sh:master Feb 19, 2022
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.

2 participants