Skip to content

Timeseries update #702

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

Merged
merged 53 commits into from
Nov 22, 2022
Merged

Timeseries update #702

merged 53 commits into from
Nov 22, 2022

Conversation

maxgrossman
Copy link
Collaborator

What I am changing

This pr fills out the api business logic to return time series data...
It also adds ui for users to interact with and download the data

How I did it

  • used good api docs in head branch plus previous materialized views to figure out sql.
  • used mock ups from lane to make the ui.
  • asked marc for advice.

How you can test it

will write these steps out in comment below...

Related Issues

@maxgrossman
Copy link
Collaborator Author

Probably will merge develop in to the head branch too so this does not look like it is change so many files...

@maxgrossman maxgrossman requested a review from kamicut May 23, 2022 18:53
immediately this fixes a flickering with header message. all around feels easier to wrangle
i clearly faked myself out when i though the upgrade worked.
when i came back this morning and rebuilt, there were some new dependencies of the upgrad using ecmascript modules that i do not think our version of next can handle. got being those being an issue to find that while the nivo bar version i selected introduces the d3-scale version with symlog, it is not used. all this to say, just use the solution from c4a666a
maxgrossman and others added 7 commits October 17, 2022 15:15
the soft semantic version was installing a version that introduced a recursive react set state bug inside the deps internal code.
pinning the version stops the bug from happening
do left join on the changeset_hashtags and changeset_countries table since it cannot be assumed every changeset will have a tracked hashtag/country
…ntries

after talking with those more familiar with osmesa stats,
query now accounts for 1 changeset having many entries in one of these tables but not all changesets having entries in these tables.
Copy link
Member

@kamicut kamicut left a comment

Choose a reason for hiding this comment

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

LGTM, but I have a couple of questions on items that might need to be removed from the PR

@@ -0,0 +1,13 @@

Copy link
Member

Choose a reason for hiding this comment

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

Why is this file here?

@kamicut
Copy link
Member

kamicut commented Nov 1, 2022

@maxgrossman This page does not work if the user accessing the Timeseries is not an admin. The /api/users call requires admin privileges. If the Timeseries is an admin feature, it should probably live in the Admin dashboard. If it isn't, then we should relax the /api/users permissions.

Copy link
Member

@kamicut kamicut left a comment

Choose a reason for hiding this comment

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

Please review the permissions for /api/users

@maxgrossman
Copy link
Collaborator Author

My vote would be keep admin now and figure out best way to relax permissions later.

As for "living in the admin dashboard" do you mean move the link to the main admin page and make the page's route something like /admin/timeseries?

@LanesGood
Copy link
Member

@maxgrossman I've been confused about something and only now understand: we don't show the data broken out as a timeseries on the timeseries page, only in the CSV download. It looks like the DevSeed team discussed this in 2020, but we didn't resolve the conversation: #603 (comment).

Rather than depicting the types of edits, and the counts, the default visualization of the timeseries data should be (as proposed by Neco) the timerseries interval bins. Tables should likewise be focused on interval bins, and likely both the chart and the table would highlight # of edits as the y-axis, and time intervals on the x-axis.

This would be a large change - can we discuss changes needed for this branch?
cc @kamicut

@kamicut kamicut merged commit 0d811fa into feature/timeseries-api Nov 22, 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.

3 participants