Set COVIDcast export values based on URL parameters #1244

rzats · 2024-03-14T22:31:29Z

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

Fills in the form values on the COVIDcast Export Data page based on URL parameters. This will be used to integrate the export UI with e.g. the EpiVis dashboard or signal documentation, with certain values being pre-filled based on parameters selected on another section of the website.

To test this out, visit the export dashboard on the preview website, then append this set of parameters to the URL:

/?data_source=covid-act-now&signal=covid-act-now-pcr_specimen_total_tests&start_day=2024-01-01&end_day=2024-02-02&geo_type=state

netlify · 2024-03-14T22:33:19Z

✅ Preview link ready!

Name	Link
🔨 Latest commit	`333a79c`
🔍 Latest deploy log	https://app.netlify.com/sites/cmu-delphi-covidcast/deploys/66043c392577910008596279
😎 Deploy Preview	https://deploy-preview-1244--cmu-delphi-covidcast.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

melange396

Not bad so far!

Date field population does not seem to be working.
We should also allow specification of as_of as well, if thats easy enough to do (ie, if its done the same way as the other date fields).
Can we use the same url parameters as the API uses? (like geo_value instead of geo_id)
Add an optional field for an api key to allow access to restricted signals (in the ui as well as in the url). The key should also be used to retrieve metadata.
The selected data source is not shown when specifying an "inactive" source (see https://deploy-preview-1244--cmu-delphi-covidcast.netlify.app/export/?source=covid-act-now&sensor=covid-act-now-pcr_specimen_total_tests&start=2024-01-01&end=2024-02-02&geo_type=state )

melange396 · 2024-03-20T17:13:44Z

clarification: the dates in the url and in the page seem to be off by one for me --

rzats · 2024-03-20T17:17:58Z

Notes from the meeting:

The initial date seems to be set fine but then the datepicker breaks - investigate that
Names for URL parameters: https://cmu-delphi.github.io/delphi-epidata/api/covidcast.html
API key URL field: put this before step 0 to get access to private signals

rzats · 2024-03-27T13:19:29Z

@melange396 fixed up all your suggestions (except the API key to allow access to restricted signals - I think that's best done in a separate PR, since it's not really related to URL params)

Here's a new URL to test out the fix to the issue you've mentioned:
https://deploy-preview-1244--cmu-delphi-covidcast.netlify.app/export/?data_source=covid-act-now&signal=covid-act-now-pcr_specimen_total_tests&start_day=2024-01-01&end_day=2024-02-02&geo_type=state

In short - this was a bug with inactive sensors in particular, where the datetime selector was incorrectly set to current (e.g. 2024) dates for inactive sensors (so e.g. only running between 2020-2021). It was possible to reset the dates to valid ones by re-selecting the date on the picker, but a bit unintuitive.

I fixed this by setting the datetime selector to the sensor's minimum and maximum start time, if it's inactive.

melange396 · 2024-03-27T15:02:07Z

Start date field is still showing off-by-one problem, end date field looks like its bound to the metadata (even though specified date is inside the valid range):

melange396 · 2024-03-27T14:36:26Z

src/modes/exportdata/ExportData.svelte

@@ -27,18 +27,55 @@
    }
  }

+  // Pre-fill the form from URL parameters.
+  // Supported parameters: source, sensor, start, end, geo_type, geo_id


change these to match new parameter names

melange396 · 2024-03-27T14:56:14Z

src/modes/exportdata/ExportData.svelte

+    // Populate region based on URL params... but let the user override this later
+    if (urlParams.has('geo_value') && !geoURLSet) {


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

melange396 · 2024-03-27T14:57:03Z

src/modes/exportdata/ExportData.svelte

+      }
+      if (
+        urlParams.has('end_day') &&
+        new Date(urlParams.get('end_day')) <= sensor.meta.minTime &&


Suggested change

new Date(urlParams.get('end_day')) <= sensor.meta.minTime &&

new Date(urlParams.get('end_day')) >= sensor.meta.minTime &&

melange396 · 2024-03-27T15:01:54Z

src/modes/exportdata/ExportData.svelte

+    if (sensor && !sensor.active) {
+      // If the sensor is inactive, set the start and end dates to its historical range rather than current date
+      if (
+        urlParams.has('start_day') &&


Im not sure we want to clamp these values to the active range if theyve been specified in the URL... The principle of least surprise suggests showing the user what they asked for.

dshemetov

This looks good enough to merge to me. Since this feature will have a "silent" release for now and none of the changes are hard to revert or modify, we can keep iterating on it live.

After syncing with Rostyslav on Slack and trying out some features out in the browser, we have a list of a few outstanding issues that Rostyslav will address in the next PR:

geo_value=CA is allowed, but geo_value=ca causes the page to hang; this is the opposite of the API behavior and needs a workaround (like dynamically lower-casing or upper-casing as needed)
when browsing on the dashboard, the URL parameters I noticed were sensor, region, and date; we will need to convert sensor into data_source and signal (should be doable with a split on "-"), region into geo_value and date into start_date / end_date

Let's make sure those tasks are captured in whatever issue is tracking this work.

dshemetov · 2024-05-13T21:04:01Z

src/modes/exportdata/ExportData.svelte

+    // Also normalize the dates to the current timezone
+    startDate = new Date(startDate.getTime() - startDate.getTimezoneOffset() * -60000);
+    endDate = new Date(endDate.getTime() - endDate.getTimezoneOffset() * -60000);


Let's add a comment about the Javascript string parsing + negative timezone edge cases here. If this is a common fix, maybe we can include a link instead of writing the full explanation out.

(stale)

melange396 · 2024-05-14T01:09:12Z

Why was this merged? There are still unaddressed concerns, plus there is no reason to release bugged and undertested functionality.

dshemetov · 2024-05-14T01:34:15Z

I thought Rostyslav addressed most of the concerns from your previous review? My personal preference here is to not let the perfect be the enemy of the good. Rostyslav made an issue on Jira to track TODOs on this feature.

dshemetov · 2024-05-14T01:51:48Z

But as a broader point, I'm not clear about our approval process for these PRs. In general, Rostyslav, we should probably wait for George's input, even if I give a more recent review. I'm writing my reviews from my personal perspective on the issue, but I'd like to give George the opportunity to give his take. I'm a bit out of the loop on things on the front-end / dev side, always happy to give input and take some burden off of George, but I do think that George has a better view on what needs to get done and when things are ready to merge.

rzats · 2024-05-15T15:09:09Z

Things to fix up:

geoURLSet - try bringing it out of the $ block so it only runs once
geo_value - convert it to uppercase internally
Add code comment about timezone conversions, especially explain the weird double negative

dshemetov · 2024-05-16T21:27:19Z

Also this one:

when browsing on the dashboard, the URL parameters I noticed were sensor, region, and date; we will need to convert sensor into data_source and signal (should be doable with a split on "-"), region into geo_value and date into start_date / end_date

rzats added 2 commits March 15, 2024 00:08

Set default values based on URL params

a621fc5

Also add geo_id

0d5d149

rzats changed the title ~~Rzatserkovnyi/export~~ Set COVIDcast export values based on URL parameters Mar 14, 2024

rzats requested a review from melange396 March 15, 2024 15:02

melange396 requested changes Mar 19, 2024

View reviewed changes

rzats added 2 commits March 27, 2024 15:03

Fix review comments

cd448c3

Discard date from URL if it's not within the right range

d9f12de

rzats requested a review from melange396 March 27, 2024 14:25

melange396 previously requested changes Mar 27, 2024

View reviewed changes

rzats added 3 commits March 27, 2024 17:16

Revert time clamping, modify comment

f978fa7

Date off-by-one error fix

b94ad01

Better timezone offset

333a79c

dshemetov approved these changes May 13, 2024

View reviewed changes

rzats requested a review from melange396 May 13, 2024 21:11

rzats merged commit d98c513 into dev May 13, 2024
6 checks passed

rzats deleted the rzatserkovnyi/export branch May 13, 2024 21:17

rzats mentioned this pull request May 22, 2024

Update COVIDcast export integration #1252

Open

4 tasks

melange396 mentioned this pull request Dec 6, 2024

Release v3.2.11 #1266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set COVIDcast export values based on URL parameters #1244

Set COVIDcast export values based on URL parameters #1244

rzats commented Mar 14, 2024 •

edited

Loading

netlify bot commented Mar 14, 2024 •

edited

Loading

melange396 left a comment

melange396 commented Mar 20, 2024

rzats commented Mar 20, 2024

rzats commented Mar 27, 2024

melange396 commented Mar 27, 2024

melange396 Mar 27, 2024

melange396 Mar 27, 2024

melange396 Mar 27, 2024

melange396 Mar 27, 2024

dshemetov left a comment •

edited

Loading

dshemetov May 13, 2024

melange396 commented May 14, 2024

dshemetov commented May 14, 2024

dshemetov commented May 14, 2024

rzats commented May 15, 2024

dshemetov commented May 16, 2024

		// Populate region based on URL params... but let the user override this later
		if (urlParams.has('geo_value') && !geoURLSet) {

	new Date(urlParams.get('end_day')) <= sensor.meta.minTime &&
	new Date(urlParams.get('end_day')) >= sensor.meta.minTime &&

Set COVIDcast export values based on URL parameters #1244

Set COVIDcast export values based on URL parameters #1244

Conversation

rzats commented Mar 14, 2024 • edited Loading

Summary

netlify bot commented Mar 14, 2024 • edited Loading

✅ Preview link ready!

melange396 left a comment

Choose a reason for hiding this comment

melange396 commented Mar 20, 2024

rzats commented Mar 20, 2024

rzats commented Mar 27, 2024

melange396 commented Mar 27, 2024

melange396 Mar 27, 2024

Choose a reason for hiding this comment

melange396 Mar 27, 2024

Choose a reason for hiding this comment

melange396 Mar 27, 2024

Choose a reason for hiding this comment

melange396 Mar 27, 2024

Choose a reason for hiding this comment

dshemetov left a comment • edited Loading

Choose a reason for hiding this comment

dshemetov May 13, 2024

Choose a reason for hiding this comment

melange396 commented May 14, 2024

dshemetov commented May 14, 2024

dshemetov commented May 14, 2024

rzats commented May 15, 2024

dshemetov commented May 16, 2024

rzats commented Mar 14, 2024 •

edited

Loading

netlify bot commented Mar 14, 2024 •

edited

Loading

dshemetov left a comment •

edited

Loading