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

Jump to bottom

fix ScheduledReports.sendReport API #17402

Merged
merged 4 commits into from Mar 30, 2021
Merged

fix ScheduledReports.sendReport API #17402

merged 4 commits into from Mar 30, 2021

Conversation

Findus23
Copy link
Member

It seems like no one (apart from @Chardonneaur) used that API in years as it always returns

{
  "result": "error",
  "message": "The period 'never' is not supported. Try any of the following instead: day, week, month, year, range"
}

even if the period is definitely not set to 'never'. It seems like the wrong variable is used here, as $report['period_param'] = $period; is set above.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@Findus23 Findus23 added the Needs Review PRs that need a code review label Mar 28, 2021
@sgiehl
Copy link
Member

sgiehl commented Mar 29, 2021

Wondering why there actually is no test that failed because of that.

@Findus23
Copy link
Member Author

I assume everything (apart from the HTTP API which isn't used by anyone) uses

matomo/plugins/ScheduledReports/ScheduledReports.php

Line 318 in fc2e8a5

public function sendReport($reportType, $report, $contents, $filename, $prettyDate, $reportSubject, $reportTitle,

directly

@tsteur
Copy link
Member

tsteur commented Mar 29, 2021

We're using this API in our app itself eg in the tasks to send every report etc. Also when clicking in the UI on "send report now" so would have assumed this worked before? Just wanting to make sure we won't regress anything

@diosmosis
Copy link
Member

@tsteur the task only supplies the report ID, not an override period. I'm writing a quick test for it now.

@diosmosis
Copy link
Member

Actually just seeing the angular controller uses the API directly w/ the period param...

@diosmosis
Copy link
Member

I see, it happens if the report's email schedule is set to a value that doesn't map to a matomo period type. (We use the correct value when generating the report, but pass the wrong value to the sendReport event; but I don't think it's used much there.)

@diosmosis diosmosis merged commit 3ab0d4e into 4.x-dev Mar 30, 2021
@diosmosis diosmosis deleted the fix-sendReport-API branch March 30, 2021 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Findus23 @sgiehl @tsteur @diosmosis