Commit c4549e910b79338c5e2df88e959fdd0127b35d45
1 parent
d0e170ee
Exists in
master
and in
1 other branch
Added simple comments form to errors. (including tests)
Showing
12 changed files
with
191 additions
and
9 deletions
Show diff stats
app/controllers/errs_controller.rb
| @@ -27,6 +27,7 @@ class ErrsController < ApplicationController | @@ -27,6 +27,7 @@ class ErrsController < ApplicationController | ||
| 27 | page = 1 if page.to_i.zero? | 27 | page = 1 if page.to_i.zero? |
| 28 | @notices = @err.notices.ordered.paginate(:page => page, :per_page => 1) | 28 | @notices = @err.notices.ordered.paginate(:page => page, :per_page => 1) |
| 29 | @notice = @notices.first | 29 | @notice = @notices.first |
| 30 | + @comment = Comment.new | ||
| 30 | end | 31 | end |
| 31 | 32 | ||
| 32 | def create_issue | 33 | def create_issue |
| @@ -62,6 +63,30 @@ class ErrsController < ApplicationController | @@ -62,6 +63,30 @@ class ErrsController < ApplicationController | ||
| 62 | redirect_to app_path(@app) | 63 | redirect_to app_path(@app) |
| 63 | end | 64 | end |
| 64 | 65 | ||
| 66 | + | ||
| 67 | + def create_comment | ||
| 68 | + @comment = Comment.new(params[:comment].merge(:user_id => current_user.id)) | ||
| 69 | + if @comment.valid? | ||
| 70 | + @err.comments << @comment | ||
| 71 | + @err.save | ||
| 72 | + flash[:success] = "Comment saved!" | ||
| 73 | + else | ||
| 74 | + flash[:error] = "I'm sorry, your comment was blank! Try again?" | ||
| 75 | + end | ||
| 76 | + redirect_to app_err_path(@app, @err) | ||
| 77 | + end | ||
| 78 | + | ||
| 79 | + def destroy_comment | ||
| 80 | + @comment = Comment.find(params[:comment_id]) | ||
| 81 | + if @comment.destroy | ||
| 82 | + flash[:success] = "Comment deleted!" | ||
| 83 | + else | ||
| 84 | + flash[:error] = "Sorry, I couldn't delete your comment for some reason. I hope you don't have any sensitive information in there!" | ||
| 85 | + end | ||
| 86 | + redirect_to app_err_path(@app, @err) | ||
| 87 | + end | ||
| 88 | + | ||
| 89 | + | ||
| 65 | protected | 90 | protected |
| 66 | 91 | ||
| 67 | def find_app | 92 | def find_app |
| @@ -83,3 +108,4 @@ class ErrsController < ApplicationController | @@ -83,3 +108,4 @@ class ErrsController < ApplicationController | ||
| 83 | end | 108 | end |
| 84 | 109 | ||
| 85 | end | 110 | end |
| 111 | + |
app/models/err.rb
| @@ -18,6 +18,7 @@ class Err | @@ -18,6 +18,7 @@ class Err | ||
| 18 | 18 | ||
| 19 | belongs_to :app | 19 | belongs_to :app |
| 20 | has_many :notices | 20 | has_many :notices |
| 21 | + has_many :comments, :inverse_of => :err, :dependent => :destroy | ||
| 21 | 22 | ||
| 22 | validates_presence_of :klass, :environment | 23 | validates_presence_of :klass, :environment |
| 23 | 24 | ||
| @@ -51,3 +52,4 @@ class Err | @@ -51,3 +52,4 @@ class Err | ||
| 51 | end | 52 | end |
| 52 | 53 | ||
| 53 | end | 54 | end |
| 55 | + |
app/views/errs/show.html.haml
| @@ -19,6 +19,23 @@ | @@ -19,6 +19,23 @@ | ||
| 19 | = link_to 'unlink issue', unlink_issue_app_err_path(@app, @err), :method => :delete, :confirm => "Unlink err issues?", :class => "unlink-issue" | 19 | = link_to 'unlink issue', unlink_issue_app_err_path(@app, @err), :method => :delete, :confirm => "Unlink err issues?", :class => "unlink-issue" |
| 20 | - if @err.unresolved? | 20 | - if @err.unresolved? |
| 21 | %span= link_to 'resolve', resolve_app_err_path(@app, @err), :method => :put, :confirm => err_confirm, :class => 'resolve' | 21 | %span= link_to 'resolve', resolve_app_err_path(@app, @err), :method => :put, :confirm => err_confirm, :class => 'resolve' |
| 22 | +- content_for :comments do | ||
| 23 | + %h3 Comments on this Err | ||
| 24 | + - @err.comments.each do |comment| | ||
| 25 | + .window | ||
| 26 | + %table.comment | ||
| 27 | + %tr | ||
| 28 | + %th | ||
| 29 | + %span= link_to '✘'.html_safe, destroy_comment_app_err_path(@app, @err) << "?comment_id=#{comment.id}", :method => :delete, :confirm => "Are sure you don't need this comment?", :class => "destroy-comment" | ||
| 30 | + = time_ago_in_words(comment.created_at, true) << " ago by " | ||
| 31 | + = link_to comment.user.email, user_path(comment.user) | ||
| 32 | + %tr | ||
| 33 | + %td= comment.body.gsub("\n", "<br>").html_safe | ||
| 34 | + | ||
| 35 | + = form_for @comment, :url => create_comment_app_err_path(@app, @err) do |comment_form| | ||
| 36 | + %p Add a comment | ||
| 37 | + = comment_form.text_area :body, :style => "width: 420px; height: 80px;" | ||
| 38 | + = comment_form.submit "Save Comment" | ||
| 22 | 39 | ||
| 23 | %h4= @notice.try(:message) | 40 | %h4= @notice.try(:message) |
| 24 | 41 | ||
| @@ -53,3 +70,4 @@ viewing occurrence #{@notices.current_page} of #{@notices.total_pages} | @@ -53,3 +70,4 @@ viewing occurrence #{@notices.current_page} of #{@notices.total_pages} | ||
| 53 | #session | 70 | #session |
| 54 | %h3 Session | 71 | %h3 Session |
| 55 | = render 'notices/session', :notice => @notice | 72 | = render 'notices/session', :notice => @notice |
| 73 | + |
app/views/layouts/application.html.haml
| @@ -28,5 +28,9 @@ | @@ -28,5 +28,9 @@ | ||
| 28 | #content | 28 | #content |
| 29 | = render :partial => 'shared/flash_messages' | 29 | = render :partial => 'shared/flash_messages' |
| 30 | = yield | 30 | = yield |
| 31 | + - if content_for?(:comments) | ||
| 32 | + #content-comments | ||
| 33 | + = yield :comments | ||
| 31 | #footer= "Powered by #{link_to 'Errbit', 'http://github.com/jdpace/errbit', :target => '_blank'}: the open source error catcher.".html_safe | 34 | #footer= "Powered by #{link_to 'Errbit', 'http://github.com/jdpace/errbit', :target => '_blank'}: the open source error catcher.".html_safe |
| 32 | = yield :scripts | 35 | = yield :scripts |
| 36 | + |
config/routes.rb
| @@ -22,6 +22,8 @@ Errbit::Application.routes.draw do | @@ -22,6 +22,8 @@ Errbit::Application.routes.draw do | ||
| 22 | put :resolve | 22 | put :resolve |
| 23 | post :create_issue | 23 | post :create_issue |
| 24 | delete :unlink_issue | 24 | delete :unlink_issue |
| 25 | + post :create_comment | ||
| 26 | + delete :destroy_comment | ||
| 25 | end | 27 | end |
| 26 | end | 28 | end |
| 27 | 29 | ||
| @@ -33,3 +35,4 @@ Errbit::Application.routes.draw do | @@ -33,3 +35,4 @@ Errbit::Application.routes.draw do | ||
| 33 | root :to => 'apps#index' | 35 | root :to => 'apps#index' |
| 34 | 36 | ||
| 35 | end | 37 | end |
| 38 | + |
public/stylesheets/application.css
| @@ -139,14 +139,17 @@ a.action { float: right; font-size: 0.9em;} | @@ -139,14 +139,17 @@ a.action { float: right; font-size: 0.9em;} | ||
| 139 | border: 1px solid #C6C6C6; | 139 | border: 1px solid #C6C6C6; |
| 140 | } | 140 | } |
| 141 | 141 | ||
| 142 | -/* Content Title */ | ||
| 143 | -#content-title { | 142 | +/* Content Title and Comments */ |
| 143 | +#content-title, #content-comments { | ||
| 144 | padding: 30px 20px; | 144 | padding: 30px 20px; |
| 145 | border-top: 1px solid #FFF; | 145 | border-top: 1px solid #FFF; |
| 146 | border-bottom: 1px solid #FFF; | 146 | border-bottom: 1px solid #FFF; |
| 147 | background-color: #e2e2e2; | 147 | background-color: #e2e2e2; |
| 148 | } | 148 | } |
| 149 | -#content-title h1 { | 149 | +#content-comments { |
| 150 | + background-color: #ffffff; | ||
| 151 | +} | ||
| 152 | +#content-title h1, #content-comments h3 { | ||
| 150 | padding: 0; margin: 0; | 153 | padding: 0; margin: 0; |
| 151 | width: 85%; | 154 | width: 85%; |
| 152 | border: none; | 155 | border: none; |
| @@ -154,6 +157,11 @@ a.action { float: right; font-size: 0.9em;} | @@ -154,6 +157,11 @@ a.action { float: right; font-size: 0.9em;} | ||
| 154 | font-size: 2em; line-height: 1em; font-weight: bold; font-family: arial, sans-serif; | 157 | font-size: 2em; line-height: 1em; font-weight: bold; font-family: arial, sans-serif; |
| 155 | word-wrap: break-word; | 158 | word-wrap: break-word; |
| 156 | } | 159 | } |
| 160 | +#content-comments h3 { | ||
| 161 | + font-size: 1.5em; | ||
| 162 | + margin-bottom: 14px; | ||
| 163 | +} | ||
| 164 | + | ||
| 157 | #content-title .meta { font-size: 0.9em; color: #787878; } | 165 | #content-title .meta { font-size: 0.9em; color: #787878; } |
| 158 | 166 | ||
| 159 | /* Action Bar */ | 167 | /* Action Bar */ |
| @@ -611,6 +619,7 @@ table.tally th.value { | @@ -611,6 +619,7 @@ table.tally th.value { | ||
| 611 | text-transform:none; | 619 | text-transform:none; |
| 612 | } | 620 | } |
| 613 | 621 | ||
| 622 | + | ||
| 614 | /* Resolve Errs */ | 623 | /* Resolve Errs */ |
| 615 | #action-bar a.resolve { | 624 | #action-bar a.resolve { |
| 616 | background: transparent url(images/icons/thumbs-up.png) 6px 5px no-repeat; | 625 | background: transparent url(images/icons/thumbs-up.png) 6px 5px no-repeat; |
| @@ -695,3 +704,28 @@ span.click_span { | @@ -695,3 +704,28 @@ span.click_span { | ||
| 695 | display: none; | 704 | display: none; |
| 696 | } | 705 | } |
| 697 | 706 | ||
| 707 | +/* Comments */ | ||
| 708 | +#content-comments form p { | ||
| 709 | + margin: 30px 0 0 0; | ||
| 710 | + text-transform: uppercase; | ||
| 711 | +} | ||
| 712 | +table.comment tbody th { | ||
| 713 | + text-transform: none; | ||
| 714 | + font-weight: normal; | ||
| 715 | + height: 20px; | ||
| 716 | + line-height: 0.5em; | ||
| 717 | +} | ||
| 718 | +table.comment tbody td { | ||
| 719 | + background-color: #F9F9F9; | ||
| 720 | +} | ||
| 721 | +#content-comments a.destroy-comment { | ||
| 722 | + color: #EE0000; | ||
| 723 | + margin-right: 5px; | ||
| 724 | +} | ||
| 725 | +#content-comments a.destroy-comment:hover { | ||
| 726 | + text-decoration: none; | ||
| 727 | +} | ||
| 728 | +#content-comments #comment_submit { | ||
| 729 | + margin-top: 15px; | ||
| 730 | +} | ||
| 731 | + |
spec/controllers/errs_controller_spec.rb
| @@ -437,4 +437,60 @@ describe ErrsController do | @@ -437,4 +437,60 @@ describe ErrsController do | ||
| 437 | end | 437 | end |
| 438 | end | 438 | end |
| 439 | end | 439 | end |
| 440 | + | ||
| 441 | + | ||
| 442 | + describe "POST /apps/:app_id/errs/:id/create_comment" do | ||
| 443 | + render_views | ||
| 444 | + | ||
| 445 | + before(:each) do | ||
| 446 | + sign_in Factory(:admin) | ||
| 447 | + end | ||
| 448 | + | ||
| 449 | + context "successful comment creation" do | ||
| 450 | + let(:err) { Factory(:err) } | ||
| 451 | + let(:user) { Factory(:user) } | ||
| 452 | + | ||
| 453 | + before(:each) do | ||
| 454 | + post :create_comment, :app_id => err.app.id, :id => err.id, | ||
| 455 | + :comment => { :body => "One test comment", :user_id => user.id } | ||
| 456 | + err.reload | ||
| 457 | + end | ||
| 458 | + | ||
| 459 | + it "should create the comment" do | ||
| 460 | + err.comments.size.should == 1 | ||
| 461 | + end | ||
| 462 | + | ||
| 463 | + it "should redirect to err page" do | ||
| 464 | + response.should redirect_to( app_err_path(err.app, err) ) | ||
| 465 | + end | ||
| 466 | + end | ||
| 467 | + end | ||
| 468 | + | ||
| 469 | + describe "DELETE /apps/:app_id/errs/:id/destroy_comment" do | ||
| 470 | + render_views | ||
| 471 | + | ||
| 472 | + before(:each) do | ||
| 473 | + sign_in Factory(:admin) | ||
| 474 | + end | ||
| 475 | + | ||
| 476 | + context "successful comment deletion" do | ||
| 477 | + let(:err) { Factory :err_with_comments } | ||
| 478 | + let(:comment) { err.comments.first } | ||
| 479 | + | ||
| 480 | + before(:each) do | ||
| 481 | + delete :destroy_comment, :app_id => err.app.id, :id => err.id, :comment_id => comment.id | ||
| 482 | + err.reload | ||
| 483 | + end | ||
| 484 | + | ||
| 485 | + it "should delete the comment" do | ||
| 486 | + err.comments.detect{|c| c.id.to_s == comment.id }.should == nil | ||
| 487 | + end | ||
| 488 | + | ||
| 489 | + it "should redirect to err page" do | ||
| 490 | + response.should redirect_to( app_err_path(err.app, err) ) | ||
| 491 | + end | ||
| 492 | + end | ||
| 493 | + end | ||
| 494 | + | ||
| 440 | end | 495 | end |
| 496 | + |
spec/factories/err_factories.rb
| @@ -4,6 +4,11 @@ Factory.define :err do |e| | @@ -4,6 +4,11 @@ Factory.define :err do |e| | ||
| 4 | e.component 'foo' | 4 | e.component 'foo' |
| 5 | e.action 'bar' | 5 | e.action 'bar' |
| 6 | e.environment 'production' | 6 | e.environment 'production' |
| 7 | + e.comments [] | ||
| 8 | +end | ||
| 9 | + | ||
| 10 | +Factory.define(:err_with_comments, :parent => :err) do |ec| | ||
| 11 | + ec.comments { (1..3).map{Factory(:comment)} } | ||
| 7 | end | 12 | end |
| 8 | 13 | ||
| 9 | Factory.define :notice do |n| | 14 | Factory.define :notice do |n| |
| @@ -22,4 +27,5 @@ def random_backtrace | @@ -22,4 +27,5 @@ def random_backtrace | ||
| 22 | 'method' => ActiveSupport.methods.shuffle.first | 27 | 'method' => ActiveSupport.methods.shuffle.first |
| 23 | }} | 28 | }} |
| 24 | backtrace | 29 | backtrace |
| 25 | -end | ||
| 26 | \ No newline at end of file | 30 | \ No newline at end of file |
| 31 | +end | ||
| 32 | + |
| @@ -0,0 +1,12 @@ | @@ -0,0 +1,12 @@ | ||
| 1 | +require 'spec_helper' | ||
| 2 | + | ||
| 3 | +describe Comment do | ||
| 4 | + context 'validations' do | ||
| 5 | + it 'should require a body' do | ||
| 6 | + comment = Factory.build(:comment, :body => nil) | ||
| 7 | + comment.should_not be_valid | ||
| 8 | + comment.errors[:body].should include("can't be blank") | ||
| 9 | + end | ||
| 10 | + end | ||
| 11 | +end | ||
| 12 | + |
spec/views/errs/show.html.haml_spec.rb
| 1 | require 'spec_helper' | 1 | require 'spec_helper' |
| 2 | 2 | ||
| 3 | -describe "errs/show.html.erb" do | ||
| 4 | - before do | 3 | +describe "errs/show.html.erb" do |
| 4 | + before do | ||
| 5 | err = Factory(:err) | 5 | err = Factory(:err) |
| 6 | + comment = Factory(:comment) | ||
| 6 | assign :err, err | 7 | assign :err, err |
| 8 | + assign :comment, comment | ||
| 7 | assign :app, err.app | 9 | assign :app, err.app |
| 8 | assign :notices, err.notices.ordered.paginate(:page => 1, :per_page => 1) | 10 | assign :notices, err.notices.ordered.paginate(:page => 1, :per_page => 1) |
| 9 | assign :notice, err.notices.first | 11 | assign :notice, err.notices.first |
| @@ -12,7 +14,7 @@ describe "errs/show.html.erb" do | @@ -12,7 +14,7 @@ describe "errs/show.html.erb" do | ||
| 12 | describe "content_for :action_bar" do | 14 | describe "content_for :action_bar" do |
| 13 | 15 | ||
| 14 | it "should confirm the 'resolve' link by default" do | 16 | it "should confirm the 'resolve' link by default" do |
| 15 | - render | 17 | + render |
| 16 | action_bar = String.new(view.instance_variable_get(:@_content_for)[:action_bar]) | 18 | action_bar = String.new(view.instance_variable_get(:@_content_for)[:action_bar]) |
| 17 | resolve_link = action_bar.match(/(<a href.*?(class="resolve").*?>)/)[0] | 19 | resolve_link = action_bar.match(/(<a href.*?(class="resolve").*?>)/)[0] |
| 18 | resolve_link.should =~ /data-confirm="Seriously\?"/ | 20 | resolve_link.should =~ /data-confirm="Seriously\?"/ |
| @@ -20,7 +22,7 @@ describe "errs/show.html.erb" do | @@ -20,7 +22,7 @@ describe "errs/show.html.erb" do | ||
| 20 | 22 | ||
| 21 | it "should confirm the 'resolve' link if configuration is unset" do | 23 | it "should confirm the 'resolve' link if configuration is unset" do |
| 22 | Errbit::Config.stub(:confirm_resolve_err).and_return(nil) | 24 | Errbit::Config.stub(:confirm_resolve_err).and_return(nil) |
| 23 | - render | 25 | + render |
| 24 | action_bar = String.new(view.instance_variable_get(:@_content_for)[:action_bar]) | 26 | action_bar = String.new(view.instance_variable_get(:@_content_for)[:action_bar]) |
| 25 | resolve_link = action_bar.match(/(<a href.*?(class="resolve").*?>)/)[0] | 27 | resolve_link = action_bar.match(/(<a href.*?(class="resolve").*?>)/)[0] |
| 26 | resolve_link.should =~ /data-confirm="Seriously\?"/ | 28 | resolve_link.should =~ /data-confirm="Seriously\?"/ |
| @@ -28,7 +30,7 @@ describe "errs/show.html.erb" do | @@ -28,7 +30,7 @@ describe "errs/show.html.erb" do | ||
| 28 | 30 | ||
| 29 | it "should not confirm the 'resolve' link if configured not to" do | 31 | it "should not confirm the 'resolve' link if configured not to" do |
| 30 | Errbit::Config.stub(:confirm_resolve_err).and_return(false) | 32 | Errbit::Config.stub(:confirm_resolve_err).and_return(false) |
| 31 | - render | 33 | + render |
| 32 | action_bar = String.new(view.instance_variable_get(:@_content_for)[:action_bar]) | 34 | action_bar = String.new(view.instance_variable_get(:@_content_for)[:action_bar]) |
| 33 | resolve_link = action_bar.match(/(<a href.*?(class="resolve").*?>)/)[0] | 35 | resolve_link = action_bar.match(/(<a href.*?(class="resolve").*?>)/)[0] |
| 34 | resolve_link.should_not =~ /data-confirm=/ | 36 | resolve_link.should_not =~ /data-confirm=/ |
| @@ -37,3 +39,4 @@ describe "errs/show.html.erb" do | @@ -37,3 +39,4 @@ describe "errs/show.html.erb" do | ||
| 37 | end | 39 | end |
| 38 | 40 | ||
| 39 | end | 41 | end |
| 42 | + |