openlibraryenvironment / reshare-analytics

Community query repository for ReShare analytics based on Metadb/LDP
Apache License 2.0
3 stars 2 forks source link

Add ability to specify start and end dates to all reports #25

Closed kristenwilson closed 1 year ago

kristenwilson commented 2 years ago

Hi @scwiek -- Would it be possible to add the ability to specify a start and end date for each of the existing queries? Let me know if you would prefer that I create separate tickets for each one. Thanks!

nassibnassar commented 2 years ago

There are two ways I know of to do this that are portable. The one I would suggest at this point is to use a common table expression (CTE) by prefixing the SELECT with something like:

WITH parameters AS (
    SELECT
        '2021-01-01' :: date AS start_date, -- start date is included in interval
        '2022-01-01' :: date AS end_date, -- end date is NOT included in interval; enter next day
)

This method should be used only for reports and not for derived tables.

julienye commented 2 years ago

I just noticed Nassib's comment that end date is NOT included in the interval. Is that the way the scripts were updated? So if a library wants to limit a report to a specific month, then the end date they specify should be the first of the next month? and not the last day of the month of interest?

If that's true, then we need to clarify that in the UI (and I will follow up with Ian)

julienye commented 2 years ago

scwiek, another question for you -- are the "division by zero" problems in core_requesting and core_supplying resolved and tested to your satisfaction? Are we ready to put those changes into production?

apologies for what may be a more cosmic question -- does the "division by zero" problem potentially exist for the other scripts, as well? did you make any changes to them?

Thanks!

scwiek commented 2 years ago

@julienye Correct, I implemented the date limiter as Nassib described, so it's an inclusive start date and an exclusive end date (which I think is how they are doing it in Folio Analytics). I believe the division by zero issue was fixed, but do let me know if you run into it while running reports on the live data. Also, I believe I addressed it in any other queries where we were calculating a ratio and a zero denominator could creep up.

nassibnassar commented 2 years ago

By default, new commits are merged to the main branch, but we also have a stable release branch where bug fix commits can also be merged; I can do that. The deployments are running the stable branch, not main.

So I think I will need to understand which commits are bug fixes (and are not introducing new features) and merge only those to the stable release branch, in order for Julie to be able to try them. Otherwise we can make a new release branch if necessary.

julienye commented 2 years ago

Thanks @scwiek for this information, good to know that you checked other queries for potential "div by zero" errors and made corrections.
This will be my first involvement in getting bug fixes released, so forgive me if I don't understand the process:

  1. @scwiek and @nassibnassar will communicate regarding getting bug fixes merged into a release branch;
  2. someone will explain when (and where) to test the bug fixes - we'll be able to test all of the scripts, yes? - could you provide a rough timeframe?
  3. @debradenault, how do you want to handle/share testing these fixes, and announcing/rolling out to PALCI and CNY?
nassibnassar commented 2 years ago

It looks like the fix was in cb702bc2920c1cbf77c4b5fa0033c2e68286b204 which I did merge into the release-0.1 branch. It has been released as v0.1.1.

debradenault commented 2 years ago

@julienye if you could take care of testing and approving fixes and let me know if all is good, then I can handle the announcing and rolling out to PALCI and CNY. Everyone once in a while we should have a meet up to see where things are at, what's outstanding etc.

julienye commented 2 years ago

@debradenault , yes I can do that moving forward. For your reference, the conversation above was regarding Sean's fix for the "division by zero" problem which we know from Nassib's comment, and from yesterday's demo, has been released.

@scwiek, what other changes are you working on ? Is there someplace I can see them, so I'll know what testing is coming my way? Thanks.

oops, sorry @sean, please ignore, I tagged the wrong Sean!