Fix the summary of converted object types to be treated as HTML #28629
Conversation
Menrath
force-pushed
the
patch-2
branch
3 times, most recently
from
10c768c
to
2e7b916
Compare
|
Thanks for your contribution. Mastodon sanitizes the HTML at a later point in the process, so you don't need to sanitize it here. However, your PR changes how the activities are rendered, without changing the test covering that behavior. They need to be updated as well. |
…etween the converted parts.
Menrath
changed the title
|
I was not too sure, whether including the '\n\n' in the test is the desired behavior. I also added a test for a more typical |
- newlines don't have to be escaped - prefer single quotes strings - fix missing space after colon
|
Sorry for all the force pushes, as you squash your pull requests anyway it would have been more clear without them to follow what happened. |
Menrath
changed the title
Menrath
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #28629 +/- ##
==========================================
+ Coverage 84.75% 84.82% +0.06%
==========================================
Files 1039 1038 -1
Lines 28175 28123 -52
Branches 4548 4536 -12
==========================================
- Hits 23881 23854 -27
+ Misses 3136 3104 -32
- Partials 1158 1165 +7 ☔ View full report in Codecov by Sentry. |
…bject with a summary.
| @@ -372,7 +372,8 @@ def in_reply_to_uri | |||
| end | |||
|
|
|||
| def converted_text | |||
| linkify([@status_parser.title.presence, @status_parser.spoiler_text.presence, @status_parser.url || @status_parser.uri].compact.join("\n\n")) | |||
| title = "<h2>#{@status_parser.title}</h2>" if @status_parser.title.present? | |||
| [title, @status_parser.spoiler_text.presence, linkify(@status_parser.url || @status_parser.uri)].compact.join("\n\n") | |||
There was a problem hiding this comment.
Minor style thing here ... since we're already in small private methods here, I might pull out a separate method for the title value here (instead of assigning local var) which would return when status_parser.title is blank, or the h2-wrapped value when its present. I'd do similar for the "url || uri" logic - and have this method be just the array of methods/values with the compact + join on the end.
|
|
||
| expect(status).to_not be_nil | ||
| expect(status.url).to eq 'https://foo.bar/@foo/1234' | ||
| expect(strip_tags(status.text)).to eq "Let's change the world\n\nWe meet on January 31st at 8pm in the FooBaar!\n\nhttps://foo.bar/@foo/1234" |
There was a problem hiding this comment.
Both this added coverage and the pre-existing are all using strip_tags in their assertions ... which I think is mostly fine for the purposes we had. That said - since this code is adding the <h2>-wrapped value ... should we assert for its presence (and/or any other ways it may be converted elsewhere?)
Tries to solve #28455
This is me the first time writing Ruby code. Also I didn't have the time to test this and setup a proper development environment. Still I wanted to try to contribute on solving this issue.
Also I am not sure where Mastodon sanitized the HTML for the content (status.text) for non-converted object types. This should also be done here.