idirlab / covid19

GNU General Public License v3.0
2 stars 0 forks source link

Default source select #17

Closed v3nd3774 closed 4 years ago

v3nd3774 commented 4 years ago

teamwork with @kmeng01 to address:

Capture

with:

ezgif-5-25c419160483

Try it out on the dev copy here.

kmeng01 commented 4 years ago

also contains the time series query as requested by @YapheeetS

824zzy commented 4 years ago

When I click up-arrow in the left side panel, it didn't jump to county level and show the data. image

Can you check which part of your codes cause this bug?

v3nd3774 commented 4 years ago

@824zzy sure thing! Good catch.

damianj commented 4 years ago

Selecting a default source other than JHU causes counties to show undefined. image For context this is what the settings look like: image

kmeng01 commented 4 years ago

@damianj Yes, because there is only county-level data from JHU. What would you like to be displayed? I can modify the API to return it

damianj commented 4 years ago

Right, but I'm only changing the state source not the county source. So shouldn't there still be values displayed there?

kmeng01 commented 4 years ago

Hmm, that seems like a frontend issue. maybe @v3nd3774 can offer more insight

kmeng01 commented 4 years ago

On second thought, this actually involves both frontend and API. We have to allow the query to specify the root default source and child default source, since the Texas default source is different from the county one.

Do we want this to be in this PR or another one? @damianj @v3nd3774

v3nd3774 commented 4 years ago

So it may be related to the data but I'm not certain yet. I pushed the latest data files that were already present on the server to git so I could use the same data that the server is using and I was able to reproduce the error in this screenshot:

Capture Is it the same error visible at the query: https://idir.uta.edu/covid-19-api-dev-2/api/v1/statquery?node=Gila%20County&date=2020-04-11&dsrc=JHU?

Capture2

v3nd3774 commented 4 years ago

@damianj I think the JHU source is still in the payload to the api as the query from the website currently shows dsrc=JHU.

kmeng01 commented 4 years ago

The problem is that the API assumes that the parent and child node use the same default source. But because we added the settings, that assumption is no longer valid

kmeng01 commented 4 years ago

I'll have to modify the API on the backend to accept both dsrc_parent and dsrc_child so that they can be differentiated. And then teh frontend must be edited to support that query

kmeng01 commented 4 years ago

Where should we make this change?

v3nd3774 commented 4 years ago

As you've suggested

v3nd3774 commented 4 years ago

If you change it to accept those parameters I can change the frontend to send them.

kmeng01 commented 4 years ago

Sure. Which branch?

v3nd3774 commented 4 years ago

Same set of acceptable string values for each of those parameters right?

You can just branch off and I'll pull your changes when you're done 👍

kmeng01 commented 4 years ago

Wait I'm not that familiar with some parts of git lol Can I just make the changes on default_source_select?

kmeng01 commented 4 years ago

And yeah, same set of string values

v3nd3774 commented 4 years ago

@kmeng01 yeah that sounds ok too

v3nd3774 commented 4 years ago

Ok @damianj I think that the changes we've made address the bug you've highlighted.

ezgif-5-6f0625c9b906

I tried it with the settings you had could you confirm if it's been addressed?

v3nd3774 commented 4 years ago

@824zzy I've made some changes to populate the parent variable in showPlace when a county is selected from the panel. I think it then flows through the code you had previously created the same way.

It results in addressing the bug you mentioned, could you confirm this is the case or let me know of a counter-example?

I tested it out below: ezgif-5-9fdaf2dad0fa

The changes should be visible at the dev endpoint.

damianj commented 4 years ago

That issue is fixed. Another issue I just noticed is at the county level when switching a state source or even leaving all on JHU, clicking "Apply changes" causes the following This image then turns into image

v3nd3774 commented 4 years ago

@damianj ok, I think it's been addressed could you take a look and confirm/deny?

damianj commented 4 years ago

lgtm

v3nd3774 commented 4 years ago

lgtm

In that case, could you merge it into master here on git?

damianj commented 4 years ago

Okay, Joey said your branch and his might have conflicts should we get those resolved first? :+1:

v3nd3774 commented 4 years ago

@damianj @kmeng01 @824zzy thanks for the hard work, the combined changes from @kmeng01 , @824zzy and myself are on the dev version. Does it look ready to merge to master?

824zzy commented 4 years ago

LGTM~