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

Update COVIDcast export integration #1252

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

rzats
Copy link
Contributor

@rzats rzats commented May 17, 2024

Followup to #1244.

Prerequisites:

  • Unless it is a hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

Fixes some pending issues from #1244:

  • Makes sure geo_value works for lowercase state names.
  • Adds code comment about timezone conversions, especially explaining the weird double negative.
  • Makes the indicator dashboard correctly integrate with the export dashboard, turning its parameters (sensor, region and date) into URL parameters. Link for testing TBD.

Testing example:

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/indicator/?sensor=hhs-confirmed_admissions_influenza_1d_prop_7dav&region=AK&date=20240426

Copy link

netlify bot commented May 17, 2024

Preview link ready!

Name Link
🔨 Latest commit c73079c
🔍 Latest deploy log https://app.netlify.com/sites/cmu-delphi-covidcast/deploys/667dafb04ecd530008c675f2
😎 Deploy Preview https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rzats rzats force-pushed the rzatserkovnyi/export-followups branch from 6ccd6d0 to 7dbe224 Compare May 22, 2024 13:39
@rzats
Copy link
Contributor Author

rzats commented May 22, 2024

@melange396 @dshemetov this is ready for review! One quick note re: #1244 (comment)

would it remove the need for geoURLSet if we put this outside of the $: {} block?

I doublechecked and that declaration relies on another variable within the $ block, so unfortunately no.

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

Hm, your testing example

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/export/?sensor=hhs-confirmed_admissions_influenza_1d_prop_7dav&region=AK&date=20240426

doesn't seem to be doing the right thing? The URL gets reduced down to

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/export/?sensor=hhs-confirmed_admissions_influenza_1d_prop_7dav

which doesn't end up selecting any parameters. Is this just an issue on my machine? I'm on Firefox. I'm expecting the URL to be reduced down to

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/export/?data_source=hhs&signal=confirmed_admissions_influenza_1d_prop_7dav&region=AK&date=20240426

does that happen your machine?

@rzats
Copy link
Contributor Author

rzats commented May 30, 2024

@dshemetov to clarify, my testing example is:

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/indicator/?sensor=hhs-confirmed_admissions_influenza_1d_prop_7dav&region=AK&date=20240426

(note the /indicator URL rather than /export)

Once on that page, click the "Export Data" link on the "about" panel and it should redirect you to the correct /export URL. (That said, if something on the website is redirecting you to the incorrect /export URL, please let me know!)

@rzats rzats requested a review from dshemetov June 21, 2024 15:52
@dshemetov
Copy link
Contributor

Hm, so when I follow your instructions (on Firefox and Chrome), the URL

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/indicator/?sensor=hhs-confirmed_admissions_influenza_1d_prop_7dav&region=AK&date=20240426

and then click Export Data, it turns into

https://deploy-preview-1252--cmu-delphi-covidcast.netlify.app/export/?sensor=hhs-confirmed_admissions_influenza_1d_prop_7dav

Does that not happen for you?

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

Still having some issues getting this to work.

import { currentMode } from '../../stores';
import AboutSection from '../../components/AboutSection.svelte';

/**
* @type {import('../../stores/params').SensorParam}
* @type {import("../stores/params").DateParam}
Copy link
Contributor

@dshemetov dshemetov Jun 21, 2024

Choose a reason for hiding this comment

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

Is the path here off? It was ../.. before? Same for the two imports below.

@melange396
Copy link
Contributor

it may not be worth adding all this complexity; the signal documentation app now has its own date/location chooser which enables it to use the /covidcast/csv endpoint directly (see cmu-delphi/signal_documentation#104), and this functionality in the covidcast export page is no longer needed.

@rzats
Copy link
Contributor Author

rzats commented Jun 23, 2024

@melange396 that makes sense for the signal documentation app! However we'd like to link to the export page from other parts of the website as well. e.g., https://delphi.cmu.edu/covidcast/indicator/ has "Export Data" links for each indicator:

image

which are intended to do what this PR and #1244 implement - redirect the user to the covidcast export page, where the form is pre-filled according to the currently selected sensor. But currently these links simply discard the ?sensor parameter (try visiting https://delphi.cmu.edu/covidcast/export?sensor=google-symptoms-s05_smoothed_search for an example).

@rzats rzats requested a review from dshemetov June 25, 2024 13:44
@dshemetov
Copy link
Contributor

dshemetov commented Jun 25, 2024

@rzats Hm, this functionality isn't working for me (same example as in my comment above, same example as in your top comment here; I tried Chrome, Firefox, Safari). Is it working for you?

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

Alright, all the buttons are working for me now. I tried a number of different sources and geos and it looks good to me. Thanks for all the iterations, Rostyslav 👍

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.

None yet

3 participants