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.

Sign up for GitHub

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: MM remote improvements #519

Merged
merged 6 commits into from
Oct 30, 2024
Merged

fix: MM remote improvements #519

merged 6 commits into from
Oct 30, 2024

Conversation

JamesMConroy
Copy link
Contributor

@JamesMConroy JamesMConroy commented Oct 8, 2024 •
edited
Loading

Proposed change

Update the MM remote so it supports

  • scheduled messages
  • output_to_rooms
  • output_to_users
  • direct messages

Types of changes

What types of changes is this pull request introducing to flottbot?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

You can fill this out after creating your PR. Put an x in the boxes that apply

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

James Conroy added 3 commits October 6, 2024 13:27
fix: the scheduler for the mattermost remote
05fc7c9 
The scheduler would never trigger, because the MM remote never populated
the list of rooms the bot can talk in.

The bot should now support the output_rooms option in rules.
fix: MM handle direct messages and output to users
9c206e1
chore: pull the direct message logic into function
c4f382c 
Helps simplify the code to call the function once instead of running
through all the steps to create a direct message every time.
@wass3r
Copy link
Collaborator

wass3r commented Oct 10, 2024

thanks for the PR, i'll review over the next few days!

@JamesMConroy
Copy link
Contributor Author

Thank you, Only thing is that I couldn't run the lint target. golangci-lint would freeze and use all my cpu.

@wass3r
Copy link
Collaborator

wass3r commented Oct 10, 2024 •
edited by wass3rw3rk
Loading

Oh, yikes. That's new. I'll see if I encounter that as well. Thanks for the heads up.

@coveralls
Copy link

coveralls commented Oct 11, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11582981152

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 11557408092: 0.0%
Covered Lines: 0
Relevant Lines: 0
💛 - Coveralls

@wass3rw3rk
Copy link
Member

the linter ran, if you want to fix up the issues from that. seems like mostly minor style issues. i will run some tests on mattermost in the meantime.

i did run golangci-lint locally (version 1.61.0). while it did take a bit of time, it did finish the checks.

thanks again for the contribution.

chore: fix some golang-ci linter issues
998576a 
stuff like cuddled assignements, unnessisary new lines, and putting the
context argument first in function parameters
@JamesMConroy
Copy link
Contributor Author

i did run golangci-lint locally (version 1.61.0). while it did take a bit of time, it did finish the checks.

I must of annoyed the computer goblins or something.

Here's what it looks like using the example scheduler rule

image

Changes are pushed. I'll probably make another MR when I want to add reactions.

@JamesMConroy
Copy link
Contributor Author

oh the channels are specified with channel/room-name and the user name shouldn't have a leading @

wass3r
wass3r requested changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to bother with such minor feedback. some super quick consistency edits and this is good to go. tested each functionality! nice work.

note to self, maybe for docs it would be good to call out that you need to prefix username with team and provide in <team>/<username> format. docs are going to get moved shortly here, so we can just create an issue for now.

remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
remote/mattermost/remote.go Outdated Show resolved Hide resolved
@wass3r wass3r mentioned this pull request Oct 24, 2024
Open
@JamesMConroy @wass3r
chore: start log messages lowercase
ee6722b 
Sentence case is not consistent with the rest of the log messages, lowercase is preferred.

Co-authored-by: david may <[email protected]>
@JamesMConroy
Copy link
Contributor Author

Minor feedback is welcome. It hopefully means there's nothing else wrong with the code.

Suggestions applied.

wass3r
wass3r approved these changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@wass3rw3rk wass3rw3rk merged commit 478989c into target:main Oct 30, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wass3r wass3r wass3r approved these changes

@wass3rw3rk wass3rw3rk wass3rw3rk approved these changes

@delta592 delta592 Awaiting requested review from delta592 delta592 is a code owner

@sjqnn sjqnn Awaiting requested review from sjqnn sjqnn is a code owner

@JordanSussman JordanSussman Awaiting requested review from JordanSussman JordanSussman is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JamesMConroy @wass3r @coveralls @wass3rw3rk