-
Notifications
You must be signed in to change notification settings - Fork 1
Initial PR to send scheduled reports to a Slack channel, #PG-4491 #1
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
Conversation
snake14
left a comment
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.
Looks alright overall. There's a lot to review. I left a few comments.
My biggest concern is whether this is going to be an open-source or premium plugin since it's in the matomo-org organisation and has GPL license throughout.
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.
This is very screenshot heavy. Could some of the test cases be DOM comparison instead? Maybe that would solve the issue of some of the test cases being tightly coupled (dependent on previous test cases)?
james-hill-matomo
left a comment
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.
I reviewed this as new code, then later realised it's based off existing plugins.
This would have been great if it was presented as a few extra PRs - e.g. one to setup up the repo with phpcs, .github etc, and a separate PR for the actual code.
I do think this makes sense as a separate repo now I've seen it.
Great work!
@snake14 I confirmed it with Stan, it is going to be an open-source plugin. |
|
@james-hill-matomo @snake14 Updated the PR description with steps to test this. |
james-hill-matomo
left a comment
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.
Looking close, only required change is a "You" -> "Your". One of us (me or Jacob) also needs to do a good local test.
james-hill-matomo
left a comment
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.
The ability to trigger an immediate sending of the report is a great feature.
I can't get it to work though. I created a new channel, and invited the bot into it.
Clicking the test button (before and after creating things in the slack) gives a successful response, but no message arrives in slack.
I added a comment in JIRA about how scheduled reports was originally aimed at a single person, which isn't really making sense as we're extending it in this direction.
|
@AltamashShaikh Please update the dashboard https://github.com/innocraft/plugin-build-dashboard |
|
@james-hill-matomo @snake14 I noticed that we need to show display options when reportType is PDF, so updated the code for that, but that won't work unless matomo-org/matomo#23546 is merged. |
Created - https://github.com/innocraft/plugin-build-dashboard/pull/3 |
james-hill-matomo
left a comment
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.
|
Has to wait for core PR |
@james-hill-matomo If I don't specify any report and description is set, I get |
* Adds PHPStan to Slack plugin * Changed the level to 8 * Bumped phpstan level up to 5 Can bump the level that high with (almost) no errors. * Don't check github action with phpstan * Test changes for multiple channels * Reverted changes * Adds the multiple channel logic back * Fixes for pre-push hook to work locally * Changes to work on all branches and reduced the phpstan level to 5 for both created and updated * Force to 8 to ensure github action is working * test for GH action * More fixes around pre-push and reverted to level 5 * Changed level to 5 for update * Get correct branch for comparison * PHPStan work correctly with DDEV * Fixed context of cd for non ddev phpstan * Changes for pre-push to work for non-dev --------- Co-authored-by: James Hill <[email protected]>
| fi | ||
| # Use ddev if setup (overridding local setup) | ||
| if command -v ddev >/dev/null 2>&1; then |
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.
This was causing issues for me since I have both php and ddev installed.
| fi | |
| # Use ddev if setup (overridding local setup) | |
| if command -v ddev >/dev/null 2>&1; then | |
| # Use ddev if setup (overridding local setup) | |
| elif command -v ddev >/dev/null 2>&1; then |
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.
@james-hill-matomo Looks good to me this suggestion, what about you ?
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.
@AltamashShaikh @snake14 I'd rather ddev was the first choice, but even more than that I'm happy to merge this as is, as it'll probably never matter anyway.
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.
Anything works for me, having ddev first should also work
|
@james-hill-matomo @snake14 Are we good to merge this PR ? We won't be releasing the plugin this week hopefully next release cycle, when we have all the FAQs and helptext ready. |
I'm OK to merge this. I've got various UI complaints (no way for users to know what a Slack Channel ID is without looking up FAQs, no feedback if you put in an invalid slack channel ID, confusing error message if you don't choose any of the Statistics Included options - but this is to the standard of the UI in general so I won't insist on fixes. |


Description
Initial PR to send scheduled reports to a Slack channel, currently we support only Slack and Email
Issue No
#PG-4491
Steps to Replicate the Issue
git clone [email protected]:matomo-org/plugin-Slack.git Slack./console plugin:activate SlackChecklist