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

Allow period used to generate scheduled report to be different from email schedule. #13832

Merged
merged 9 commits into from May 13, 2019

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Dec 12, 2018

Simple solution that does not support range periods.

Refs #13819

@diosmosis diosmosis added the Needs Review PRs that need a code review label Dec 12, 2018
@tsteur
Copy link
Member

tsteur commented Dec 13, 2018

Can we list the schedule first and then report period? and add some inline help for report period? also I would maybe automatically set report period to the same value when changing the email schedule but not the other way around. By default they should always match unless someone changes report period. If they changed report period, and then change the schedule, I would again also set report period to the same as email schedule.(no need to remember whether it was different I think, but could maybe... not sure which is better).

@diosmosis
Copy link
Member Author

👍 will do

@diosmosis
Copy link
Member Author

Updated the PR. @tsteur let me know if this can go in 3.8

@diosmosis diosmosis added this to the 3.9.0 milestone Feb 2, 2019
@sgiehl
Copy link
Member

sgiehl commented Feb 25, 2019

@diosmosis this one needs a rebase to update the version and the update script name

@diosmosis
Copy link
Member Author

Updated.

@mattab mattab modified the milestones: 3.9.0, 3.10.0 Mar 18, 2019
@tsteur
Copy link
Member

tsteur commented Apr 11, 2019

Looks good to merge, we only need to update version number and update script. Feel free to merge if tests pass 👍

@diosmosis diosmosis merged commit 4b81b3b into 3.x-dev May 13, 2019
@diosmosis diosmosis deleted the 13819-period-reports branch May 13, 2019 23:46
@mattab
Copy link
Member

mattab commented Jun 29, 2019

Seeing this late and commenting late, but is this feature any useful? I understand the use case in #13819 "I would like to send periodically (say "weekly") an email report, and the report should include data over (say) the past 12 months.". But in its current form the feature does not let users do this. I can't think of any use case for it. Should we maybe revert the PR to not make the scheduled reports harder to use? or is there some use case i'm missing? @tsteur @diosmosis

@mattab
Copy link
Member

mattab commented Jun 29, 2019

Actually i can think of a use case, where you can now use this feature to receive "Daily" the "Current month" report (or "Current week") report. Which seems valuable 👍
Would this use case work? (haven't tested)

@diosmosis
Copy link
Member Author

diosmosis commented Jun 29, 2019

That would work. This was mostly for getting weekly email of daily report (for the system report in cloud since we don't want to sum each day up for that; only the daily report is accurate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants