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

pkp/pkp-lib#6171 Remove editorial stats not used in OPS #69

Merged
merged 4 commits into from
Oct 20, 2020

Conversation

ajnyga
Copy link
Contributor

@ajnyga ajnyga commented Oct 7, 2020

Step 1 for refactorin editorial stats in OPS: remove unnecessary statistics
pkp/pkp-lib#6171

@asmecher
Copy link
Member

asmecher commented Oct 7, 2020

Thanks, @ajnyga! Hmm, I think rather than coding the applications to remove specific functionality that they shouldn't use ("destructive" customization), I'd prefer "constructive" customization -- maybe adding a feature into the pkp-lib that allows each app to specify what features (e.g. stats) they do want to include. That'll be less prone to breakage, I think.

@ajnyga
Copy link
Contributor Author

ajnyga commented Oct 7, 2020

Thanks. I actually discussed this issue with @NateWr in Slack. He was open for both solutions, using the hook to edit the array values or to override the whole class in OPS. Both are fine by me.

@ajnyga
Copy link
Contributor Author

ajnyga commented Oct 7, 2020

Oh, sorry I read your solution poorly. Sure I can do that, does @NateWr have some thoughts for this?

@NateWr
Copy link
Contributor

NateWr commented Oct 8, 2020

I'd prefer "constructive" customization -- maybe adding a feature into the pkp-lib that allows each app to specify what features (e.g. stats) they do want to include.

Generally, I agree. I think that it will be a little bit trickier in this case. It's not that easy to isolate each statistic. Many of the stats rely on other stats being fetched and calculated first (for example, the acceptance rate expects the number of accepted and received to have been calculated). So in addition to having each application define what stats it wants, we'd also need to code in the dependency logic (when A is requested, also fetch B and C), which I think is tricky to maintain across three applications.

In this case, I prefer more of a brute force approach to customization. Although it may result in more updates that span more than one application, each of these updates will be simpler to implement.

I think Alec is right that relying on symbolic identifiers to remove items is prone to breakage. One caveat here is that these keys are unique identifiers in the REST API, so they're documented and shouldn't change without documentation. But he's right that there's a tight coupling here based on symbolic identifiers that is maybe not the best idea.

PKPStatsEditorialService::getOverview() is not a complex method, so OPS could overwrite that entirely with very little effort. Maybe that's the best way to go. Sorry for the misdirection, @ajnyga.

@ajnyga
Copy link
Contributor Author

ajnyga commented Oct 12, 2020

fyi @asmecher @NateWr going with "PKPStatsEditorialService::getOverview() is not a complex method, so OPS could overwrite that entirely with very little effort." A new pr later today.

@ajnyga
Copy link
Contributor Author

ajnyga commented Oct 20, 2020

ping @NateWr and/or @asmecher cleaned the code with Nate's help and tests are running.

As I said above, this is the initial step for OPS editorial stats that leaves the most central stats intact. Step 2 will be adding new stats there, but for this we need a discussion. I will start a new issue once the original one is closed when this pr is merged.

@asmecher asmecher merged commit 37476df into pkp:master Oct 20, 2020
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