Some more fixes #145

Closed
wants to merge 13 commits into
from

4 participants

@Flameeyes

Okay this is a bunch of different fixes that I applied to my blog — the akismet one is not live on my blog but it's tested, I just don't want to run the migration myself, until it's in master.

The gravatar change could break IE6 on some rare cases when accessing it through https using SNI certificates, but that's such a corner case that I honestly don't care.

HTH,
Diego

Flameeyes added some commits Feb 17, 2013
@Flameeyes Flameeyes admin/feedback: make sure that "Mark checked item as (Ham|Spam)" do w…
…hat they say.

The behaviour of these was to just toggle the state, now they are
actually marking them as ham/spam so that they don't misbehave.
1c09ab7
@Flameeyes Flameeyes akismet: save the user agent and submit it when checking for/submitti…
…ng spam.

The User-Agent sent by the commenter is often a good piece of
information to identify whether a comment is spam or not, so make sure
not to blackhole it and instead store and submit it.
0f9582e
@Flameeyes Flameeyes gravatar: use protocol-relative URIs for HTTPS compatibility. c051dfc
@Flameeyes Flameeyes Fix text filter settings on editing/autosaving old posts and pages. 2f864a5
@Flameeyes Flameeyes flickr: make use of protocol-relative URIs on the returned source data.
Also, for lightbox, make sure that it uses fully relative paths for
the stylesheets and lightbox as otherwise it can also escape the HTTPS
protocol.
49f5cc1
@mvz mvz and 1 other commented on an outdated diff Feb 17, 2013
...o_textfilter_lightbox/lib/typo_textfilter_lightbox.rb
@@ -123,8 +125,8 @@ def self.macrofilter(blog,content,attrib,params,text="")
def self.set_whiteboard(blog, content)
content.whiteboard['page_header_lightbox'] = <<-HTML
- <link href="#{blog.base_url}/stylesheets/lightbox.css" media="all" rel="Stylesheet" type="text/css" />
- <script src="#{blog.base_url}/javascripts/lightbox.js" type="text/javascript"></script>
+ <link href="/stylesheets/lightbox.css" media="all" rel="Stylesheet" type="text/css" />
+ <script src="/javascripts/lightbox.js" type="text/javascript"></script>
@mvz
mvz added a line comment Feb 17, 2013

This breaks if the blog is not located at the root URI. It might be better to use javascript_include_tag and stylesheet_link_tag.

@Flameeyes
Flameeyes added a line comment Feb 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mvz

These are some great fixes. I may be able to take a look at the javascript and stylesheet tags in the lightbox plugin later in the week.

@Flameeyes
Flameeyes added some commits Feb 17, 2013
@Flameeyes Flameeyes comments: use relative URLs for comment forms.
Using full, absoltue URLs will cause testing environment to comment on
production instances, and will also break HTTPS support on
mixed-protocols sites.
f28c8b6
@Flameeyes Flameeyes Remove executable permissions to files that shouldn't be executed. 9043a05
@Flameeyes Flameeyes Fix links to relative paths instead of absolute paths.
This unbreaks the view on a local test system and supports HTTPS more
properly.
3bff4ad
@Flameeyes Flameeyes excerpts: pre-process the excerpts with the filter
This allows posts with markup early in the body to not show said markup.
58f615f
@Flameeyes

Do you have a non-root install to try this method on? It should work fine.

@xenji

I would like to see those patches reviewed and in the master as soon as possible. I'm about to start a fixing-round on my own and I do not want to do the work in double ... ;)

@mvz

@Flameeyes Yes, I have a non-root install for testing.
@xenji I should be able to test and merge this weekend. Still need to go over the new commits.

@mvz

@Flameeyes did you mean for the opengraph commit to be part of this pull request? GitHub automatically adds any commits on the same branch to the pull request ...

@Flameeyes

@mvz yeah do you find anything against it? It provides very basic information but at least this way the links look correctly on Facebook with it.

@mvz

Interesting ... no I have no problem with it. I don't really use Facebook myself so wouldn't know what 'correctly' means in this case.

@mvz

It turns out this breaks the specs pretty badly and actually keeps the text filter at textile unless set through params.

How does it break it exactly? Besides, I honestly care less about the specs than the actual behaviour. Current master is TFU in this regard, as editing always reset to none.

Article sets text_filter to textile automatically, so the calls to set_textfilter never happen. This will also happen in production, so this means set_textfilter can just be removed.

With failing specs there is nothing to fall back on in development, so I'm not going to merge anything in with specs failing.

Do as you wish, but know that right now the thing is messed up in production.

Maybe I should just give up on Typo, if specs are more important than behaviour.

@mvz

This causes as failing spec

@mvz

I'm sorry, but there are just too many things that break right now. Some involve fixes to the specs to match improved behavior, and I can do these myself, but the changes to lightbox cause several of the lightbox specs to break in a way that suggests it breaks in production as well.

I do agree wholeheartedly that the text filter bug needs to be fixed ASAP.

@Flameeyes

I've asked to see how the specs break and you haven't pasted a single line of output, so I'm afraid that if you want to accept the fixes and figure out why the specs break, it's your call. I really won't care about whether Typo upstream merges this or not at this point: on my blog it works. That includes lightbox, which I even used again today.

Honestly, you should consider whether to make it easier to run those specs, and whether specs are more important than what the user gets or not. Good luck.

@mvz

Huh? Is there a particular difficulty with running the specs that we are not aware of?

@mvz

BTW, you can now freely apply your own migrations, since Typo now uses time stamped migrations (although no new migrations have been generated since the switch.

Below is the output of the broken tests. Note that I had to merge in current master to make the tests run at all. Perhaps this will also solve your inability to run the tests.

Failures:

  1) Admin::ContentController with admin connection it should behave like new action should create a filtered article
     Failure/Error: new_article.text_filter.name.should eq @user.text_filter.name

       expected: "markdown"
            got: "textile"

       (compared using ==)
     Shared Example Group: "new action" called from ./spec/controllers/admin/content_controller_spec.rb:486
     # ./spec/controllers/admin/content_controller_spec.rb:350:in `block (3 levels) in <top (required)>'

  2) Admin::ContentController with publisher connection it should behave like new action should create a filtered article
     Failure/Error: new_article.text_filter.name.should eq @user.text_filter.name

       expected: "markdown"
            got: "textile"

       (compared using ==)
     Shared Example Group: "new action" called from ./spec/controllers/admin/content_controller_spec.rb:632
     # ./spec/controllers/admin/content_controller_spec.rb:350:in `block (3 levels) in <top (required)>'

  3) String#to_title should handle the case where item.body is nil
     Failure/Error: item.should_receive(:body).and_return(nil)
       (Double "item").body(any args)
           expected: 1 time
           received: 0 times
     # ./spec/lib/transforms_spec.rb:8:in `block (3 levels) in <top (required)>'

  4) Article#comment_url should render complete url of comment
     Failure/Error: article.comment_url.should == "http://myblog.net/comments?article_id=#{article.id}"
       expected: "http://myblog.net/comments?article_id=123"
            got: "/comments?article_id=123" (using ==)
     # ./spec/models/article_spec.rb:472:in `block (3 levels) in <top (required)>'

  5) Article#preview_comment_url should render complete url of comment
     Failure/Error: article.preview_comment_url.should == "http://myblog.net/comments/preview?article_id=#{article.id}"
       expected: "http://myblog.net/comments/preview?article_id=123"
            got: "/comments/preview?article_id=123" (using ==)
     # ./spec/models/article_spec.rb:479:in `block (3 levels) in <top (required)>'

  6) With the list of available filters #filter_text specific typo tags flickr should show with default settings
     Failure/Error: assert_equal "<div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>",
     MiniTest::Assertion:
       <"<div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>"> expected but was
       <"<div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"//photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>">.
     # ./spec/models/text_filter_spec.rb:80:in `block (5 levels) in <top (required)>'

  7) With the list of available filters #filter_text specific typo tags flickr should use default image size
     Failure/Error: assert_equal "<div style=\"\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>",
     MiniTest::Assertion:
       <"<div style=\"\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>"> expected but was
       <"<div style=\"\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"//photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>">.
     # ./spec/models/text_filter_spec.rb:87:in `block (5 levels) in <top (required)>'

  8) With the list of available filters #filter_text specific typo tags flickr should use caption
     Failure/Error: assert_equal "<div style=\"\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a></div>",
     MiniTest::Assertion:
       <"<div style=\"\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a></div>"> expected but was
       <"<div style=\"\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"//photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a></div>">.
     # ./spec/models/text_filter_spec.rb:94:in `block (5 levels) in <top (required)>'

  9) With the list of available filters #filter_text lightbox should work
     Failure/Error: assert_equal "<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_b.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_t.jpg\" width=\"67\" height=\"100\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:67px\">This is Matz, Ruby's creator</p>",
     MiniTest::Assertion:
       <"<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_b.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_t.jpg\" width=\"67\" height=\"100\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:67px\">This is Matz, Ruby's creator</p>"> expected but was
       <"<typo:lightbox img=\"31366117\" thumbsize=\"Thumbnail\" displaysize=\"Large\" style=\"float:left\"/>">.
     # ./spec/models/text_filter_spec.rb:189:in `block (4 levels) in <top (required)>'

  10) With the list of available filters #filter_text lightbox shoudl use default thumb image size
     Failure/Error: assert_equal "<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_b.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p>",
     MiniTest::Assertion:
       <"<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_b.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p>"> expected but was
       <"<typo:lightbox img=\"31366117\" displaysize=\"Large\"/>">.
     # ./spec/models/text_filter_spec.rb:196:in `block (4 levels) in <top (required)>'

  11) With the list of available filters #filter_text lightbox should use default display image size
     Failure/Error: assert_equal "<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_o.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p>",
     MiniTest::Assertion:
       <"<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_o.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p>"> expected but was
       <"<typo:lightbox img=\"31366117\"/>">.
     # ./spec/models/text_filter_spec.rb:203:in `block (4 levels) in <top (required)>'

  12) With the list of available filters #filter_text lightbox should work with caption
     Failure/Error: assert_equal "<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_o.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a>",
     MiniTest::Assertion:
       <"<a href=\"http://photos23.flickr.com/31366117_b1a791d68e_o.jpg\" rel=\"lightbox\" title=\"Matz\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a>"> expected but was
       <"<typo:lightbox img=\"31366117\" caption=\"\"/>">.
     # ./spec/models/text_filter_spec.rb:210:in `block (4 levels) in <top (required)>'

  13) With the list of available filters #filter_text combining a post-macro with markdown correctly interprets the macro
     Failure/Error: result.should =~ %r{<div style="float:left" class="flickrplugin"><a href="http://www.flickr.com/users/scottlaird/31366117"><img src="http://photos23.flickr.com/31366117_b1a791d68e_s.jpg" width="75" height="75" alt="Matz" title="Matz"/></a><p class="caption" style="width:75px">This is Matz, Ruby's creator</p></div>}
       expected: /<div style="float:left" class="flickrplugin"><a href="http:\/\/www.flickr.com\/users\/scottlaird\/31366117"><img src="http:\/\/photos23.flickr.com\/31366117_b1a791d68e_s.jpg" width="75" height="75" alt="Matz" title="Matz"\/><\/a><p class="caption" style="width:75px">This is Matz, Ruby's creator<\/p><\/div>/
            got: "<p><div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"//photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div></p>" (using =~)
       Diff:
       @@ -1,2 +1,2 @@
       -/<div style="float:left" class="flickrplugin"><a href="http:\/\/www.flickr.com\/users\/scottlaird\/31366117"><img src="http:\/\/photos23.flickr.com\/31366117_b1a791d68e_s.jpg" width="75" height="75" alt="Matz" title="Matz"\/><\/a><p class="caption" style="width:75px">This is Matz, Ruby's creator<\/p><\/div>/
       +"<p><div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"//photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div></p>"

     # ./spec/models/text_filter_spec.rb:222:in `block (5 levels) in <top (required)>'

  14) With the list of available filters #filter_text combining a post-macro with markdown correctly interprets the macro
     Failure/Error: result.should == "<div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>"
       expected: "<div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"http://photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>"
            got: "<div style=\"float:left\" class=\"flickrplugin\"><a href=\"http://www.flickr.com/users/scottlaird/31366117\"><img src=\"//photos23.flickr.com/31366117_b1a791d68e_s.jpg\" width=\"75\" height=\"75\" alt=\"Matz\" title=\"Matz\"/></a><p class=\"caption\" style=\"width:75px\">This is Matz, Ruby's creator</p></div>" (using ==)
     # ./spec/models/text_filter_spec.rb:230:in `block (5 levels) in <top (required)>'

Finished in 3 minutes 37.31 seconds
1248 examples, 14 failures, 7 pending

Failed examples:

rspec ./spec/controllers/admin/content_controller_spec.rb:338 # Admin::ContentController with admin connection it should behave like new action should create a filtered article
rspec ./spec/controllers/admin/content_controller_spec.rb:338 # Admin::ContentController with publisher connection it should behave like new action should create a filtered article
rspec ./spec/lib/transforms_spec.rb:6 # String#to_title should handle the case where item.body is nil
rspec ./spec/models/article_spec.rb:470 # Article#comment_url should render complete url of comment
rspec ./spec/models/article_spec.rb:477 # Article#preview_comment_url should render complete url of comment
rspec ./spec/models/text_filter_spec.rb:79 # With the list of available filters #filter_text specific typo tags flickr should show with default settings
rspec ./spec/models/text_filter_spec.rb:86 # With the list of available filters #filter_text specific typo tags flickr should use default image size
rspec ./spec/models/text_filter_spec.rb:93 # With the list of available filters #filter_text specific typo tags flickr should use caption
rspec ./spec/models/text_filter_spec.rb:188 # With the list of available filters #filter_text lightbox should work
rspec ./spec/models/text_filter_spec.rb:195 # With the list of available filters #filter_text lightbox shoudl use default thumb image size
rspec ./spec/models/text_filter_spec.rb:202 # With the list of available filters #filter_text lightbox should use default display image size
rspec ./spec/models/text_filter_spec.rb:209 # With the list of available filters #filter_text lightbox should work with caption
rspec ./spec/models/text_filter_spec.rb:219 # With the list of available filters #filter_text combining a post-macro with markdown correctly interprets the macro
rspec ./spec/models/text_filter_spec.rb:227 # With the list of available filters #filter_text combining a post-macro with markdown correctly interprets the macro
@brodock

any update on this issue?

@Flameeyes

Doubt so, I'm not going to spend much time on trying to fix specs that are vastly broken by design, and the rest of the Typo team does not seem to care about fixing their stuff.

I actually have a couple more fixes, mostly to use a modern version of the Gravatar API, but for the most part I only care about re-applying my fixes on top of master nowadays, from time to time.

@brodock

I can do the heavy work of fixing specs... I just want to have some directions to what fix first. I've noticed by what mvz posted here, that there are specs that are wrong by default... if someone from the core team could just create a simple "todo" list of what have to be changed I can do it.

I'm asking that because I don't have that much time to figure out that information by myself, but want to contribute some time doing actual useful work.

@mvz

@Flameeyes can you please tell me what needs fixing? I have provided output of the failing tests and have not heard back since. There's a lot wrong with the specs, mostly because they're an accretion of 7 years of different insights into TDD/BDD. Unfortunately, it's all we have, and doing a massive overhaul is a lot of work.

As to this branch: The text filter settings issue has been fixed differently in master, but the other fixes are still worth merging. I can cherry-pick the commits that apply cleanly and leave the ones that break the specs. Then, we can see what still needs to be done.

@mvz mvz added a commit that referenced this pull request Apr 30, 2013
@mvz mvz Merge branch 'several-fixes'
Merges in most of pull request #145.
eaa038c
@mvz

I was able to cherry-pick most of these commits by applying simple fixes to the specs, except for the following commits:

  • 49f5cc1: This breaks the lightbox filter because the #base_blog_url controller method cannot be accessed from the filter.
  • 2f864a5: The text filter issues were fixed differently in master.
@mvz
mvz commented Jun 9, 2013

No further information received. I'm done here.

@mvz mvz closed this Jun 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment