-
Notifications
You must be signed in to change notification settings - Fork 9
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
Timeseries update #702
Conversation
Probably will merge develop in to the head branch too so this does not look like it is change so many files... |
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
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.
There was a problem hiding this 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
api/src/user_badges_clock.js
Outdated
@@ -0,0 +1,13 @@ | |||
|
There was a problem hiding this comment.
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?
@maxgrossman This page does not work if the user accessing the Timeseries is not an admin. The |
There was a problem hiding this 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
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 |
Co-authored-by: Lane Goodman <lane@developmentseed.org>
update timeseries data using submit button
Restrict timeseries chart to top 20 users
Timeseries UI UX Enhancements
Make timeseries an admin area page
@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? |
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
How you can test it
will write these steps out in comment below...
Related Issues