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

comments on profile page fixed #2197

Merged
merged 4 commits into from
Feb 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/answers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def delete
current_user.role == 'admin' ||
current_user.role == 'moderator'
respond_to do |format|
if @answer.delete
if @answer.destroy
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy method triggers dependent: :destroy callback, hence, delete was changed to destroy here.

format.html { redirect_to @answer.node.path(:question), notice: 'Answer deleted' }
format.js
else
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/notes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ def delete
@node = Node.find(params[:id])
if current_user && (current_user.uid == @node.uid || current_user.role == "moderator" || current_user.role == "admin")
if @node.authors.uniq.length == 1
@node.delete
@node.destroy
respond_with do |format|
format.html do
if request.xhr?
Expand Down
2 changes: 1 addition & 1 deletion app/models/answer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class Answer < ActiveRecord::Base
belongs_to :node, foreign_key: 'nid', dependent: :destroy
belongs_to :drupal_user, foreign_key: 'uid'
has_many :answer_selections, foreign_key: 'aid'
has_many :comments, foreign_key: 'aid'
has_many :comments, foreign_key: 'aid', dependent: :destroy

validates :content, presence: true

Expand Down
6 changes: 3 additions & 3 deletions app/models/comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ class Comment < ActiveRecord::Base
:subject, :hostname, :comment,
:status, :format, :thread, :timestamp

belongs_to :node, foreign_key: 'nid', touch: true,
dependent: :destroy, counter_cache: true
belongs_to :node, foreign_key: 'nid', touch: true, counter_cache: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, does this have to do with the comment changes? What was the issue? I wasn't sure counter_cache was working and would like to demonstrate this with a test if that's OK... a unit test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jywarren. Actually I haven't removed counter_cache but 'dependent: :destroy'.
Actually the error was occurring because some comments did not belong to an answer or a node. Perhaps there is no need of dependent: :destroy here but the node model (has_many comments, dependent: :destroy). Just realized that dependent destroy also needs to be added to answer model here:-

has_many :comments, foreign_key: 'aid'

Please correct me if I am wrong!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jywarren , The tests are written. Please have a look

# dependent: :destroy, counter_cache: true
belongs_to :drupal_user, foreign_key: 'uid'
belongs_to :answer, foreign_key: 'aid'

Expand Down Expand Up @@ -67,7 +67,7 @@ def parent
if aid == 0
node
else
answer.node
return answer.node unless answer.nil?
end
end

Expand Down
2 changes: 1 addition & 1 deletion app/models/node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def updated_month
has_many :tag, through: :node_tag
# these override the above... have to do it manually:
# has_many :tag, :through => :drupal_node_tag
has_many :comments, foreign_key: 'nid' #, dependent: :destroy # re-enable in Rails 5
has_many :comments, foreign_key: 'nid' , dependent: :destroy # re-enable in Rails 5
has_many :drupal_content_type_map, foreign_key: 'nid' #, dependent: :destroy # re-enable in Rails 5
has_many :drupal_content_field_mappers, foreign_key: 'nid' #, dependent: :destroy # re-enable in Rails 5
has_many :drupal_content_field_map_editor, foreign_key: 'nid' #, dependent: :destroy # re-enable in Rails 5
Expand Down
7 changes: 7 additions & 0 deletions test/unit/answer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,11 @@ class AnswerTest < ActiveSupport::TestCase
answer = answers(:one)
assert_equal answer.comments.last, Comment.last
end

test 'should delete associated comments when an answer is deleted' do
answer = answers(:one)
assert_equal answer.comments.count, 2
deleted_answer = answer.destroy
assert_equal answer.comments.count, 0
end
end
7 changes: 7 additions & 0 deletions test/unit/node_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -336,4 +336,11 @@ class NodeTest < ActiveSupport::TestCase
assert_not_nil tagged_note
assert_equal jeff_notes, tagged_note
end

test 'should delete associated comments when a node is deleted' do
node = nodes(:one)
assert_equal node.comments.count, 4
deleted_node = node.destroy
assert_equal node.comments.count, 0
end
end