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

Change NotifySlack payload #412

Merged
merged 3 commits into from
Jul 31, 2021
Merged

Change NotifySlack payload #412

merged 3 commits into from
Jul 31, 2021

Conversation

Quarky9
Copy link
Contributor

@Quarky9 Quarky9 commented Jul 30, 2021

to use new blocks feature to allow more markdown parsing and formatting.

Description:

Changed the payload format of Slack from old attachment to new recommended blocks attachment.
Incl. sections for body ( title optional as header section ) and footer as context block.

See: https://api.slack.com/reference/messaging/payload under attachments.

I kept the outer attachment structure though for its support of the coloured bar.

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • No lint errors (use flake8)
  • 100% test coverage -

All Slack related tests passed.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2021

Codecov Report

Merging #412 (1ef8a4e) into master (ab6b6b5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #412   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           94        94           
  Lines        11966     11968    +2     
  Branches      2011      2012    +1     
=========================================
+ Hits         11966     11968    +2     
Impacted Files Coverage Δ
apprise/plugins/NotifySlack.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab6b6b5...1ef8a4e. Read the comment docs.

@caronc
Copy link
Owner

caronc commented Jul 31, 2021

Thanks for your pull request! 👍 🙏
Very much appreciated!

@caronc caronc merged commit 49af85b into caronc:master Jul 31, 2021
@artbycrunk
Copy link

@Quarky9
seems like this change has bug where the default icon is no longer being passed, as following code was removed

if image_url:
                payload['icon_url'] = image_url

@Quarky9
Copy link
Contributor Author

Quarky9 commented Oct 25, 2021

@artbycrunk That is correct but according to my tests it does not make a difference, details see https://api.slack.com/methods/chat.postMessage#arg_icon_url
Also Slack is changing the APIs constantly and we are in legacy territory with our current payload as far as I understand.
Happy to be corrected though ...
What is the issue you are facing / difference ?

@artbycrunk
Copy link

@Quarky9 Yeah so in my case I pass a different icon_url passed on the type of msg I am posting to a channel, and after this change, I noticed that it default to the icon set for the channel.
When debugging locally I added the "icon_url" back to the payload and that was more in line with the previous behaviour.

@Quarky9
Copy link
Contributor Author

Quarky9 commented Oct 26, 2021

Hi,
yeah actually I am doing the same thing and the image for each type ends up in the footer element of the payload and shows in each message. And the actual user avatar cannot be changed for a bot normally anymore anyways.

As adding it back in does not affect my tests at least I will send another PR to re-add the line when I get to it.
Also adding @caronc for visibility.

@artbycrunk
Copy link

@Quarky9 thanks for the quick fix

@caronc
Copy link
Owner

caronc commented Nov 11, 2021

This new format seems to have undesirable effects. I"m wonder if there is a way we can blend the old way with your suggested way a bit better?

@Quarky9
Copy link
Contributor Author

Quarky9 commented Nov 11, 2021

@caronc During my changes, I kept the way of nesting the actual message within the top-level 'attachments' structure to preserve the original style incl. the coloured bars etc.
Details: https://api.slack.com/reference/messaging/payload

We might want to try to also add the top-level 'text' element next to 'attachments' and see if that fixes the issues mentioned in
#473

@caronc
Copy link
Owner

caronc commented Nov 11, 2021

I like your idea; I was experimenting already before you had replied and had a small PR which keeps your changes (and i tested all 4 combinations) and it works well.

It just allows mobile stuff to work out of the gate, at the same time you still get to leverage the style you prefer.

@Quarky9
Copy link
Contributor Author

Quarky9 commented Nov 12, 2021

Tested both versions ( blocks yes/no).
Looks good apart from the HTML encoding for markdown as also commented in the PR itself.

@caronc
Copy link
Owner

caronc commented Nov 12, 2021

I had the escape break for the title, but no matter what i did for URL testing (using the block) the escaping worked fine. So i put that in if-block. The website kind of hints that you need to escape the content to avoid contents since it uses those keywords internally.

@Quarky9
Copy link
Contributor Author

Quarky9 commented Nov 12, 2021

In general escaping is probably the right thing but within the blocks and when you want to actually use markdown and eg. links escaping breaks it. See https://api.slack.com/reference/surfaces/formatting#linking-urls as example.
I guess when Slack says "text objects" they mean plain text only not markdown.

@caronc
Copy link
Owner

caronc commented Nov 12, 2021

Okay, I'll leave the escaping on for the legacy mode only then? Would that work better maybe?

@Quarky9
Copy link
Contributor Author

Quarky9 commented Nov 12, 2021

Ideally we only escape the fields which are text vs. skip escaping for anything being markdn

@Quarky9
Copy link
Contributor Author

Quarky9 commented Nov 12, 2021

@caronc Tested your latest commit. Works like a charm same as before using ?blocks=yes as well as old behaviour on my end.
👍

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.

4 participants