Commit 5c82d114300e01166275df2d402317b40061084d
Exists in
staging
and in
2 other branches
Merge branch 'comment_paragraph_performance' into staging
Conflicts: plugins/comment_paragraph/controllers/profile/comment_paragraph_plugin_profile_controller.rb plugins/comment_paragraph/test/functional/comment_paragraph_plugin_profile_controller_test.rb
Showing
10 changed files
with
91 additions
and
12 deletions
Show diff stats
features/step_definitions/web_steps.rb
@@ -43,14 +43,29 @@ When /^(?:|I )follow "([^"]*)"(?: within "([^"]*)")?$/ do |link, selector| | @@ -43,14 +43,29 @@ When /^(?:|I )follow "([^"]*)"(?: within "([^"]*)")?$/ do |link, selector| | ||
43 | click_link(link, :match => :prefer_exact) | 43 | click_link(link, :match => :prefer_exact) |
44 | rescue Selenium::WebDriver::Error::UnknownError => selenium_error | 44 | rescue Selenium::WebDriver::Error::UnknownError => selenium_error |
45 | if selenium_error.message.start_with? 'Element is not clickable at point' | 45 | if selenium_error.message.start_with? 'Element is not clickable at point' |
46 | - href = find_link(link)[:href] | 46 | + link = find_link(link) |
47 | + href = link[:href] | ||
48 | + onclick = link[:onClick] | ||
47 | 49 | ||
48 | warn "#{selenium_error.message}\n\n"\ | 50 | warn "#{selenium_error.message}\n\n"\ |
49 | - "Trying to overcome this by redirecting you to the link's href:\n"\ | ||
50 | - "\t'#{href}'\n\n"\ | ||
51 | - "Good luck and be careful that this may produce hidden links to work on tests!\n" | 51 | + "Trying to overcome this by:\n" |
52 | 52 | ||
53 | - visit href | 53 | + onclick_return = true |
54 | + | ||
55 | + unless onclick.nil? | ||
56 | + warn "\t* Running onClick JS:\n"\ | ||
57 | + "\t\t'#{onclick}'\n" | ||
58 | + onclick_return = page.execute_script onclick | ||
59 | + end | ||
60 | + | ||
61 | + if onclick_return | ||
62 | + warn "\t* Redirecting you to the link's href:\n"\ | ||
63 | + "\t\t'#{href}'\n" | ||
64 | + | ||
65 | + visit href | ||
66 | + end | ||
67 | + | ||
68 | + warn "\nGood luck and be careful that this may produce hidden links to work on tests!\n" | ||
54 | else | 69 | else |
55 | raise selenium_error | 70 | raise selenium_error |
56 | end | 71 | end |
@@ -277,7 +292,18 @@ Then /^display "([^\"]*)"$/ do |element| | @@ -277,7 +292,18 @@ Then /^display "([^\"]*)"$/ do |element| | ||
277 | end | 292 | end |
278 | 293 | ||
279 | Then /^I fill in tinyMCE "(.*?)" with "(.*?)"$/ do |field, content| | 294 | Then /^I fill in tinyMCE "(.*?)" with "(.*?)"$/ do |field, content| |
280 | - execute_script("$(tinymce.editors['#{field}'].setContent('#{content}'))") | 295 | + n = 0 |
296 | + begin | ||
297 | + execute_script("tinymce.editors['#{field}'].setContent('#{content}')") | ||
298 | + rescue Selenium::WebDriver::Error::JavascriptError | ||
299 | + n += 1 | ||
300 | + if n < 5 | ||
301 | + sleep 1 | ||
302 | + retry | ||
303 | + else | ||
304 | + raise | ||
305 | + end | ||
306 | + end | ||
281 | end | 307 | end |
282 | 308 | ||
283 | Then /^there should be a div with class "([^"]*)"$/ do |klass| | 309 | Then /^there should be a div with class "([^"]*)"$/ do |klass| |
lib/feed_handler.rb
@@ -60,7 +60,7 @@ class FeedHandler | @@ -60,7 +60,7 @@ class FeedHandler | ||
60 | def process(container) | 60 | def process(container) |
61 | begin | 61 | begin |
62 | container.class.transaction do | 62 | container.class.transaction do |
63 | - if container.update_errors > FeedHandler.max_errors && container.fetched_at < (Time.now - FeedHandler.disabled_period) | 63 | + if failed_too_many_times(container) && enough_time_since_last_failure(container) |
64 | container.enabled = true | 64 | container.enabled = true |
65 | container.update_errors = 0 | 65 | container.update_errors = 0 |
66 | container.save | 66 | container.save |
@@ -110,4 +110,12 @@ class FeedHandler | @@ -110,4 +110,12 @@ class FeedHandler | ||
110 | url =~ URI.regexp('http') || url =~ URI.regexp('https') | 110 | url =~ URI.regexp('http') || url =~ URI.regexp('https') |
111 | end | 111 | end |
112 | 112 | ||
113 | + def failed_too_many_times(container) | ||
114 | + container.update_errors > FeedHandler.max_errors | ||
115 | + end | ||
116 | + | ||
117 | + def enough_time_since_last_failure(container) | ||
118 | + container.fetched_at.nil? || container.fetched_at < (Time.now - FeedHandler.disabled_period) | ||
119 | + end | ||
120 | + | ||
113 | end | 121 | end |
plugins/comment_paragraph/controllers/profile/comment_paragraph_plugin_profile_controller.rb
1 | require 'csv' | 1 | require 'csv' |
2 | -class CommentParagraphPluginProfileController < ProfileController | 2 | +class CommentParagraphPluginProfileController < CommentController |
3 | append_view_path File.join(File.dirname(__FILE__) + '/../../views') | 3 | append_view_path File.join(File.dirname(__FILE__) + '/../../views') |
4 | 4 | ||
5 | def view_comments | 5 | def view_comments |
@@ -12,6 +12,16 @@ class CommentParagraphPluginProfileController < ProfileController | @@ -12,6 +12,16 @@ class CommentParagraphPluginProfileController < ProfileController | ||
12 | render :partial => 'comment/comment.html.erb', :collection => @comments | 12 | render :partial => 'comment/comment.html.erb', :collection => @comments |
13 | end | 13 | end |
14 | 14 | ||
15 | + def comment_form | ||
16 | + @page = profile.articles.find(params[:article_id]) | ||
17 | + render :partial => 'comment/comment_form', :locals => { | ||
18 | + :comment => Comment.new, | ||
19 | + :display_link => true, | ||
20 | + :cancel_triggers_hide => true, | ||
21 | + :paragraph_uuid => params[:paragraph_uuid] | ||
22 | + } | ||
23 | + end | ||
24 | + | ||
15 | include CommentParagraphPlugin::CommentsReport | 25 | include CommentParagraphPlugin::CommentsReport |
16 | 26 | ||
17 | def export_comments | 27 | def export_comments |
plugins/comment_paragraph/lib/comment_paragraph_plugin/macros/allow_comment.rb
@@ -10,7 +10,8 @@ class CommentParagraphPlugin::AllowComment < Noosfero::Plugin::Macro | @@ -10,7 +10,8 @@ class CommentParagraphPlugin::AllowComment < Noosfero::Plugin::Macro | ||
10 | def parse(params, inner_html, source) | 10 | def parse(params, inner_html, source) |
11 | paragraph_uuid = params[:paragraph_uuid] | 11 | paragraph_uuid = params[:paragraph_uuid] |
12 | article = source | 12 | article = source |
13 | - count = article.paragraph_comments.without_spam.in_paragraph(paragraph_uuid).count | 13 | + @paragraph_comments_counts ||= article.paragraph_comments.without_spam.group(:paragraph_uuid).reorder(:paragraph_uuid).count |
14 | + count = @paragraph_comments_counts.fetch(paragraph_uuid, 0) | ||
14 | 15 | ||
15 | proc { | 16 | proc { |
16 | if controller.kind_of?(ContentViewerController) && article.comment_paragraph_plugin_activated? | 17 | if controller.kind_of?(ContentViewerController) && article.comment_paragraph_plugin_activated? |
plugins/comment_paragraph/lib/ext/article.rb
@@ -36,7 +36,7 @@ class Article | @@ -36,7 +36,7 @@ class Article | ||
36 | if body && (body_changed? || setting_changed?(:comment_paragraph_plugin_activate)) | 36 | if body && (body_changed? || setting_changed?(:comment_paragraph_plugin_activate)) |
37 | updated = body_changed? ? body_change[1] : body | 37 | updated = body_changed? ? body_change[1] : body |
38 | doc = Nokogiri::HTML(updated) | 38 | doc = Nokogiri::HTML(updated) |
39 | - doc.css('li, body > div, body > span, body > p').each do |paragraph| | 39 | + (doc.css('li') + doc.css('body > div, body > span, body > p')).each do |paragraph| |
40 | next if paragraph.css('[data-macro="comment_paragraph_plugin/allow_comment"]').present? || paragraph.content.blank? | 40 | next if paragraph.css('[data-macro="comment_paragraph_plugin/allow_comment"]').present? || paragraph.content.blank? |
41 | 41 | ||
42 | commentable = Nokogiri::XML::Node.new("span", doc) | 42 | commentable = Nokogiri::XML::Node.new("span", doc) |
plugins/comment_paragraph/public/comment_paragraph_macro.js
@@ -80,6 +80,12 @@ jQuery(document).ready(function($) { | @@ -80,6 +80,12 @@ jQuery(document).ready(function($) { | ||
80 | container.find('.display-comment-form').show(); | 80 | container.find('.display-comment-form').show(); |
81 | } | 81 | } |
82 | }); | 82 | }); |
83 | + var formDiv = container.find('.side-comment .post_comment_box'); | ||
84 | + if(formDiv.find('.page-comment-form').length==0) { | ||
85 | + $.ajax(formDiv.data('comment_paragraph_form_url')).done(function(data) { | ||
86 | + formDiv.append(data); | ||
87 | + }); | ||
88 | + } | ||
83 | }); | 89 | }); |
84 | 90 | ||
85 | 91 |
plugins/comment_paragraph/test/functional/comment_paragraph_plugin_profile_controller_test.rb
@@ -10,6 +10,9 @@ class CommentParagraphPluginProfileControllerTest < ActionController::TestCase | @@ -10,6 +10,9 @@ class CommentParagraphPluginProfileControllerTest < ActionController::TestCase | ||
10 | @profile = create_user('testuser').person | 10 | @profile = create_user('testuser').person |
11 | @article = profile.articles.build(:name => 'test') | 11 | @article = profile.articles.build(:name => 'test') |
12 | @article.save! | 12 | @article.save! |
13 | + @environment = Environment.default | ||
14 | + @environment.enabled_plugins = ['CommentParagraphPlugin'] | ||
15 | + @environment.save! | ||
13 | end | 16 | end |
14 | attr_reader :article, :profile | 17 | attr_reader :article, :profile |
15 | 18 | ||
@@ -39,6 +42,14 @@ class CommentParagraphPluginProfileControllerTest < ActionController::TestCase | @@ -39,6 +42,14 @@ class CommentParagraphPluginProfileControllerTest < ActionController::TestCase | ||
39 | assert_match /d comment/, @response.body | 42 | assert_match /d comment/, @response.body |
40 | end | 43 | end |
41 | 44 | ||
45 | + should 'load the comment form for a paragraph' do | ||
46 | + login_as('testuser') | ||
47 | + comment = fast_create(Comment, :source_id => article, :author_id => profile, :title => 'a comment', :body => 'lalala', :paragraph_uuid => 0) | ||
48 | + xhr :get, :comment_form, :profile => @profile.identifier, :article_id => article.id, :paragraph_uuid => 0 | ||
49 | + assert_select ".page-comment-form" | ||
50 | + assert_select "#comment_paragraph_uuid[value=?]", '0' | ||
51 | + end | ||
52 | + | ||
42 | should 'export comments as CSV' do | 53 | should 'export comments as CSV' do |
43 | comment1 = fast_create(Comment, :created_at => Time.now - 1.days, :source_id => article, :author_id => profile, :title => 'a comment', :body => 'a comment', :paragraph_uuid => nil) | 54 | comment1 = fast_create(Comment, :created_at => Time.now - 1.days, :source_id => article, :author_id => profile, :title => 'a comment', :body => 'a comment', :paragraph_uuid => nil) |
44 | comment2 = fast_create(Comment, :created_at => Time.now - 2.days, :source_id => article, :author_id => profile, :title => 'b comment', :body => 'b comment', :paragraph_uuid => nil) | 55 | comment2 = fast_create(Comment, :created_at => Time.now - 2.days, :source_id => article, :author_id => profile, :title => 'b comment', :body => 'b comment', :paragraph_uuid => nil) |
plugins/comment_paragraph/test/unit/allow_comment_test.rb
@@ -46,4 +46,13 @@ class AllowCommentTest < ActiveSupport::TestCase | @@ -46,4 +46,13 @@ class AllowCommentTest < ActiveSupport::TestCase | ||
46 | assert_equal 'inner', instance_eval(&content) | 46 | assert_equal 'inner', instance_eval(&content) |
47 | end | 47 | end |
48 | 48 | ||
49 | + should 'preload comment counts when parsing content' do | ||
50 | + 3.times { fast_create(Comment, :paragraph_uuid => '2', :source_id => article.id) } | ||
51 | + content = macro.parse({:paragraph_uuid => comment.paragraph_uuid}, article.body, article) | ||
52 | + paragraph_comments_counts = macro.instance_variable_get(:@paragraph_comments_counts) | ||
53 | + assert_equivalent ['1', '2'], paragraph_comments_counts.keys | ||
54 | + assert_equal 1, paragraph_comments_counts['1'] | ||
55 | + assert_equal 3, paragraph_comments_counts['2'] | ||
56 | + end | ||
57 | + | ||
49 | end | 58 | end |
plugins/comment_paragraph/views/comment_paragraph_plugin_profile/_comment_paragraph.html.erb
@@ -11,14 +11,14 @@ | @@ -11,14 +11,14 @@ | ||
11 | </div> | 11 | </div> |
12 | 12 | ||
13 | <% load_comments_url = url_for({:profile => profile_identifier, :controller => 'comment_paragraph_plugin_profile', :action => 'view_comments', :paragraph_uuid => paragraph_uuid, :article_id => article_id}) %> | 13 | <% load_comments_url = url_for({:profile => profile_identifier, :controller => 'comment_paragraph_plugin_profile', :action => 'view_comments', :paragraph_uuid => paragraph_uuid, :article_id => article_id}) %> |
14 | + <% load_comment_form_url = url_for({:profile => profile_identifier, :controller => 'comment_paragraph_plugin_profile', :action => 'comment_form', :paragraph_uuid => paragraph_uuid, :article_id => article_id}) %> | ||
14 | 15 | ||
15 | <div class="side-comment" data-comment_paragraph_url="<%= load_comments_url %>"> | 16 | <div class="side-comment" data-comment_paragraph_url="<%= load_comments_url %>"> |
16 | <div class="article-comments-list"> | 17 | <div class="article-comments-list"> |
17 | <div class="loading"></div> | 18 | <div class="loading"></div> |
18 | </div> | 19 | </div> |
19 | <div class ="article-comments-list-more"></div> | 20 | <div class ="article-comments-list-more"></div> |
20 | - <div class='post_comment_box closed'> | ||
21 | - <%= render :partial => 'comment/comment_form', :locals => {:comment => Comment.new, :display_link => true, :cancel_triggers_hide => true, :paragraph_uuid => paragraph_uuid}%> | 21 | + <div class='post_comment_box closed' data-comment_paragraph_form_url="<%= load_comment_form_url %>"> |
22 | </div> | 22 | </div> |
23 | </div> | 23 | </div> |
24 | </div> | 24 | </div> |
test/unit/feed_handler_test.rb
@@ -133,6 +133,14 @@ class FeedHandlerTest < ActiveSupport::TestCase | @@ -133,6 +133,14 @@ class FeedHandlerTest < ActiveSupport::TestCase | ||
133 | 133 | ||
134 | assert container.enabled, 'must reenable container after <disabled_period> (%s)' % container_class | 134 | assert container.enabled, 'must reenable container after <disabled_period> (%s)' % container_class |
135 | end | 135 | end |
136 | + | ||
137 | + should "handle a feed that was never fetched successfully (#{container_class})" do | ||
138 | + container = create(container_class) | ||
139 | + container.update_errors = FeedHandler.max_errors + 1 | ||
140 | + container.fetched_at = nil | ||
141 | + handler.expects(:actually_process_container).with(container) | ||
142 | + handler.process(container) | ||
143 | + end | ||
136 | end | 144 | end |
137 | 145 | ||
138 | should 'not crash even when finish fetch fails' do | 146 | should 'not crash even when finish fetch fails' do |