From 6f67b580c489c5a95abd6e57e5a4e0b3ccc47a05 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Mon, 22 Jul 2013 15:27:14 +0400 Subject: [PATCH 01/10] #222: added form for creating new branch --- .../controllers/project_refs_controller.js | 17 +++++++++++++++++ .../angularjs/models/project_ref.js | 1 + .../javascripts/angularjs/services/project.js | 5 +++++ app/assets/stylesheets/design/custom.scss | 5 ++++- .../projects/git/trees_controller.rb | 9 +++++++-- .../projects/git/trees/branches.html.haml | 18 ++++++++++++++---- config/locales/models/project.en.yml | 1 + config/locales/models/project.ru.yml | 1 + config/routes.rb | 1 + lib/modules/models/git.rb | 8 ++++++++ 10 files changed, 59 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/angularjs/controllers/project_refs_controller.js b/app/assets/javascripts/angularjs/controllers/project_refs_controller.js index a7b7e1ffa..512341851 100644 --- a/app/assets/javascripts/angularjs/controllers/project_refs_controller.js +++ b/app/assets/javascripts/angularjs/controllers/project_refs_controller.js @@ -7,6 +7,7 @@ RosaABF.controller('ProjectRefsController', ['$scope', '$http', 'ApiProject', fu $scope.project_id = null; $scope.current_ref = null; $scope.project_resource = null; + $scope.new_branch = false; $scope.init = function(project_id, ref, locale) { $scope.project_id = project_id; @@ -49,6 +50,22 @@ RosaABF.controller('ProjectRefsController', ['$scope', '$http', 'ApiProject', fu $scope.singleton.project.branches_count = $scope.branches.length; } + $scope.create = function(branch) { + branch.ui_container = false; + $scope.project_resource.$create_branch( + { + owner: $scope.project.owner.uname, + project: $scope.project.name, + from_ref: branch.ref, + new_ref: branch.new_ref + }, function() { // on success + $scope.getRefs(); + }, function () { // on error + $scope.getRefs(); + } + ); + } + $scope.destroy = function(branch) { $scope.project_resource.$delete_branch( {owner: $scope.project.owner.uname, project: $scope.project.name, ref: branch.ref}, diff --git a/app/assets/javascripts/angularjs/models/project_ref.js b/app/assets/javascripts/angularjs/models/project_ref.js index b13b40dc9..8e8258374 100644 --- a/app/assets/javascripts/angularjs/models/project_ref.js +++ b/app/assets/javascripts/angularjs/models/project_ref.js @@ -11,6 +11,7 @@ var ProjectRef = function(atts) { //with some logic... self.isTag = self.object.type == 'tag'; + self.ui_container = false; self.path = function(project) { return '/' + project.fullname + '/tree/' + self.ref; diff --git a/app/assets/javascripts/angularjs/services/project.js b/app/assets/javascripts/angularjs/services/project.js index 66a55dff2..3b34e4617 100644 --- a/app/assets/javascripts/angularjs/services/project.js +++ b/app/assets/javascripts/angularjs/services/project.js @@ -18,6 +18,11 @@ RosaABF.factory("ApiProject", ['$resource', function($resource) { url: '/:owner/:project/branches/:ref', // ?sha= method: 'PUT', isArray : false + }, + create_branch: { + url: '/:owner/:project/branches', // ?new_ref=&from_ref= + method: 'POST', + isArray : false } } ); diff --git a/app/assets/stylesheets/design/custom.scss b/app/assets/stylesheets/design/custom.scss index 76a7224b2..7d5e43824 100644 --- a/app/assets/stylesheets/design/custom.scss +++ b/app/assets/stylesheets/design/custom.scss @@ -75,7 +75,10 @@ header menu ul li a { td.actions ul { float: right; } - .text { font-size: 12px; } + .text { + font-size: 12px; + padding-top: 2px; + } td { border-bottom: 1px solid #a9c6dd; padding: 0 20px; diff --git a/app/controllers/projects/git/trees_controller.rb b/app/controllers/projects/git/trees_controller.rb index 9cb34a9ba..27e2a7d0d 100644 --- a/app/controllers/projects/git/trees_controller.rb +++ b/app/controllers/projects/git/trees_controller.rb @@ -4,8 +4,8 @@ class Projects::Git::TreesController < Projects::Git::BaseController skip_before_filter :set_branch_and_tree, :set_treeish_and_path, :only => :archive before_filter lambda { raise Grit::NoSuchPathError if params[:treeish] != @branch.try(:name) }, :only => [:branch, :destroy] - skip_authorize_resource :project, :only => [:destroy, :restore_branch] - before_filter lambda { authorize!(:write, @project) }, :only => [:destroy, :restore_branch] + skip_authorize_resource :project, :only => [:destroy, :restore_branch, :create] + before_filter lambda { authorize!(:write, @project) }, :only => [:destroy, :restore_branch, :create] def show render('empty') and return if @project.is_empty? @@ -39,6 +39,11 @@ class Projects::Git::TreesController < Projects::Git::BaseController render :nothing => true end + def create + status = @project.create_branch(params[:new_ref], params[:from_ref]) ? 200 : 422 + render :nothing => true, :status => status + end + def destroy status = @branch && @project.delete_branch(@branch, current_user) ? 200 : 422 render :nothing => true, :status => status diff --git a/app/views/projects/git/trees/branches.html.haml b/app/views/projects/git/trees/branches.html.haml index 912207c01..c05770993 100644 --- a/app/views/projects/git/trees/branches.html.haml +++ b/app/views/projects/git/trees/branches.html.haml @@ -22,13 +22,23 @@ %a{'ng-href' => '{{branch.path(project)}}' } {{branch.ref}} %td.actions %ul.actions - %li.text{'ng-show' => 'branch.ref == current_ref'} - = t('layout.projects.base_branch') - if can?(:write, @project) - %li{'ng-hide' => 'branch.ref == current_ref'} + %li{'ng-hide' => 'branch.ref == current_ref || branch.ui_container'} %a{:href => '', 'ng-click' => 'destroy(branch)'} = t('layout.projects.delete_branch') - %li{'ng-hide' => 'branch.ref == current_ref'} + %li{'ng-hide' => 'branch.ui_container'} + %a{:href => '', 'ng-click' => 'branch.ui_container = true'} + = t('layout.projects.new_branch') + %li{'ng-show' => 'branch.ui_container'} + %form{'ng-submit' => 'create(branch)'} + %input{:name => 'new_ref', 'ng-model' => 'branch.new_ref'} + %input{:type => 'submit', :value =>t('layout.create')} + %a{:href => '', 'ng-click' => 'branch.ui_container = false'} + = t('layout.cancel') + %li{'ng-hide' => 'branch.ref == current_ref || branch.ui_container'} %a{'ng-href' => '{{branch.diff_path(project, current_ref)}}' } = t('layout.projects.compare') + %li.text{'ng-show' => 'branch.ref == current_ref && !branch.ui_container'} + = t('layout.projects.base_branch') + diff --git a/config/locales/models/project.en.yml b/config/locales/models/project.en.yml index 1492f8fb9..bf018c003 100644 --- a/config/locales/models/project.en.yml +++ b/config/locales/models/project.en.yml @@ -28,6 +28,7 @@ en: new_build_list: New build confirm_delete: Are you sure you want to delete this project? new: New project + new_branch: New branch location: Location git_repo_location: Path to git repo current_project_header: Current project diff --git a/config/locales/models/project.ru.yml b/config/locales/models/project.ru.yml index a647bb2b7..7f9cfc5fc 100644 --- a/config/locales/models/project.ru.yml +++ b/config/locales/models/project.ru.yml @@ -28,6 +28,7 @@ ru: new_build_list: Новая сборка confirm_delete: Вы уверены, что хотите удалить этот проект? new: Новый проект + new_branch: Новая ветка location: Расположение git_repo_location: Путь к git-репозиторию current_project_header: Текущий проект diff --git a/config/routes.rb b/config/routes.rb index be0045a48..942acad29 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -337,6 +337,7 @@ Rosa::Application.routes.draw do get '/branches/:treeish' => "git/trees#branches", :as => :branches delete '/branches/:treeish' => "git/trees#destroy", :as => :branches put '/branches/:treeish' => "git/trees#restore_branch", :as => :branches + post '/branches' => "git/trees#create", :as => :branches # Commits get '/commits/:treeish(/*path)' => "git/commits#index", :as => :commits, :format => false get '/commit/:id(.:format)' => "git/commits#show", :as => :commit diff --git a/lib/modules/models/git.rb b/lib/modules/models/git.rb index 2de1d4b58..496d12c7d 100644 --- a/lib/modules/models/git.rb +++ b/lib/modules/models/git.rb @@ -36,6 +36,14 @@ module Modules # TODO: return something else instead of empty string on success and error def restore_branch(branch, sha) repo.git.native(:branch, {}, branch, sha) + return true + end + + def create_branch(new_ref, from_ref) + return false if new_ref.blank? || from_ref.blank? || + repo.branches.none?{|b| b.name == from_ref} || + repo.branches.one?{|b| b.name == new_ref} + restore_branch new_ref, from_ref end def delete_branch(branch, user) From 0c1334aef7d1c6fdd4246fdcd715cdf5b1f3543a Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Mon, 22 Jul 2013 16:13:07 +0400 Subject: [PATCH 02/10] #222: added specs for GitTree#create --- .../projects/git/git_trees_controller_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/controllers/projects/git/git_trees_controller_spec.rb b/spec/controllers/projects/git/git_trees_controller_spec.rb index f2eb72079..eb73a0ed8 100644 --- a/spec/controllers/projects/git/git_trees_controller_spec.rb +++ b/spec/controllers/projects/git/git_trees_controller_spec.rb @@ -47,6 +47,11 @@ describe Projects::Git::TreesController do response.should_not be_success end + it 'should not be able to perform create action' do + post :create, @params.merge(:treeish => '', :from_ref => 'master', :new_ref => 'master-1') + response.should_not be_success + end + end context 'for other user' do @@ -80,6 +85,11 @@ describe Projects::Git::TreesController do response.should_not be_success end + it 'should not be able to perform create action' do + post :create, @params.merge(:treeish => '', :from_ref => 'master', :new_ref => 'master-1') + response.should_not be_success + end + [:tags, :branches].each do |action| it "should be able to perform #{action} action" do get action, @params.merge(:treeish => 'master') @@ -109,6 +119,11 @@ describe Projects::Git::TreesController do put :restore_branch, @params.merge(:treeish => 'conflicts') response.should be_success end + + it 'should be able to perform create action' do + post :create, @params.merge(:treeish => '', :from_ref => 'master', :new_ref => 'master-1') + response.should be_success + end end after(:all) {clean_projects_dir} From 6f748e9bafad23fbd5b1535adff8dd1d85a4949c Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Mon, 22 Jul 2013 16:17:19 +0400 Subject: [PATCH 03/10] #222: removed unnecessary code --- .../javascripts/angularjs/controllers/project_refs_controller.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/javascripts/angularjs/controllers/project_refs_controller.js b/app/assets/javascripts/angularjs/controllers/project_refs_controller.js index 512341851..f471ebc63 100644 --- a/app/assets/javascripts/angularjs/controllers/project_refs_controller.js +++ b/app/assets/javascripts/angularjs/controllers/project_refs_controller.js @@ -7,7 +7,6 @@ RosaABF.controller('ProjectRefsController', ['$scope', '$http', 'ApiProject', fu $scope.project_id = null; $scope.current_ref = null; $scope.project_resource = null; - $scope.new_branch = false; $scope.init = function(project_id, ref, locale) { $scope.project_id = project_id; From 079f8a955a15e0dd8a479d790f1b9555459a6d59 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Mon, 22 Jul 2013 18:21:12 +0400 Subject: [PATCH 04/10] #222: init GitHook on creating branch from UI --- app/controllers/projects/git/trees_controller.rb | 6 +++--- lib/modules/models/git.rb | 15 ++++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/app/controllers/projects/git/trees_controller.rb b/app/controllers/projects/git/trees_controller.rb index 27e2a7d0d..b04a78688 100644 --- a/app/controllers/projects/git/trees_controller.rb +++ b/app/controllers/projects/git/trees_controller.rb @@ -35,12 +35,12 @@ class Projects::Git::TreesController < Projects::Git::BaseController end def restore_branch - @project.restore_branch @treeish, params[:sha] if @treeish.present? && params[:sha].present? - render :nothing => true + status = @project.restore_branch(@treeish, params[:sha], current_user) ? 200 : 422 + render :nothing => true, :status => status end def create - status = @project.create_branch(params[:new_ref], params[:from_ref]) ? 200 : 422 + status = @project.create_branch(params[:new_ref], params[:from_ref], current_user) ? 200 : 422 render :nothing => true, :status => status end diff --git a/lib/modules/models/git.rb b/lib/modules/models/git.rb index 496d12c7d..619d27a7d 100644 --- a/lib/modules/models/git.rb +++ b/lib/modules/models/git.rb @@ -34,23 +34,24 @@ module Modules end # TODO: return something else instead of empty string on success and error - def restore_branch(branch, sha) + def restore_branch(branch, sha, user) + return false if branch.blank? || sha.blank? repo.git.native(:branch, {}, branch, sha) + Resque.enqueue(GitHook, owner.uname, name, sha, GitHook::ZERO, "refs/heads/#{branch}", 'commit', "user-#{user.id}", nil) return true end - def create_branch(new_ref, from_ref) - return false if new_ref.blank? || from_ref.blank? || - repo.branches.none?{|b| b.name == from_ref} || - repo.branches.one?{|b| b.name == new_ref} - restore_branch new_ref, from_ref + def create_branch(new_ref, from_ref, user) + branch = repo.branches.detect{|b| b.name == from_ref} + return false if !branch || repo.branches.one?{|b| b.name == new_ref} + restore_branch new_ref, branch.commit.id, user end def delete_branch(branch, user) return false if default_branch == branch.name message = repo.git.native(:branch, {}, '-D', branch.name) if message.present? - Resque.enqueue(GitHook,owner.uname, name, GitHook::ZERO, branch.commit.id, "refs/heads/#{branch.name}", 'commit', "user-#{user.id}", message) + Resque.enqueue(GitHook, owner.uname, name, GitHook::ZERO, branch.commit.id, "refs/heads/#{branch.name}", 'commit', "user-#{user.id}", message) end return message.present? end From c935120398d27968e41ea99f8d2a7350e2369305 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Mon, 22 Jul 2013 18:38:18 +0400 Subject: [PATCH 05/10] #222: Added checking of sha according to Alexander's comment --- lib/modules/models/git.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/modules/models/git.rb b/lib/modules/models/git.rb index 619d27a7d..5770f30c4 100644 --- a/lib/modules/models/git.rb +++ b/lib/modules/models/git.rb @@ -35,7 +35,7 @@ module Modules # TODO: return something else instead of empty string on success and error def restore_branch(branch, sha, user) - return false if branch.blank? || sha.blank? + return false if branch.blank? || sha.blank? || repo.commit(sha).nil? repo.git.native(:branch, {}, branch, sha) Resque.enqueue(GitHook, owner.uname, name, sha, GitHook::ZERO, "refs/heads/#{branch}", 'commit', "user-#{user.id}", nil) return true From f69013329a0259c75d1371e561b9a57584e88e1e Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Mon, 22 Jul 2013 21:21:51 +0400 Subject: [PATCH 06/10] #222: removed unnecessary attrs --- app/views/projects/git/trees/branches.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/git/trees/branches.html.haml b/app/views/projects/git/trees/branches.html.haml index c05770993..500d6988a 100644 --- a/app/views/projects/git/trees/branches.html.haml +++ b/app/views/projects/git/trees/branches.html.haml @@ -31,7 +31,7 @@ = t('layout.projects.new_branch') %li{'ng-show' => 'branch.ui_container'} %form{'ng-submit' => 'create(branch)'} - %input{:name => 'new_ref', 'ng-model' => 'branch.new_ref'} + %input{'ng-model' => 'branch.new_ref'} %input{:type => 'submit', :value =>t('layout.create')} %a{:href => '', 'ng-click' => 'branch.ui_container = false'} = t('layout.cancel') From c9b7f890fa6ea5e9b558eef633b211047ef7b90f Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Tue, 23 Jul 2013 17:29:41 +0400 Subject: [PATCH 07/10] #222: removed #restore_branch method --- .../projects/git/trees_controller.rb | 2 +- lib/modules/models/git.rb | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/app/controllers/projects/git/trees_controller.rb b/app/controllers/projects/git/trees_controller.rb index b04a78688..e34fd8167 100644 --- a/app/controllers/projects/git/trees_controller.rb +++ b/app/controllers/projects/git/trees_controller.rb @@ -35,7 +35,7 @@ class Projects::Git::TreesController < Projects::Git::BaseController end def restore_branch - status = @project.restore_branch(@treeish, params[:sha], current_user) ? 200 : 422 + status = @project.create_branch(@treeish, params[:sha], current_user) ? 200 : 422 render :nothing => true, :status => status end diff --git a/lib/modules/models/git.rb b/lib/modules/models/git.rb index 5770f30c4..032d8bb57 100644 --- a/lib/modules/models/git.rb +++ b/lib/modules/models/git.rb @@ -33,18 +33,15 @@ module Modules repo.tags.map(&:name) + repo.branches.map(&:name) end - # TODO: return something else instead of empty string on success and error - def restore_branch(branch, sha, user) - return false if branch.blank? || sha.blank? || repo.commit(sha).nil? - repo.git.native(:branch, {}, branch, sha) - Resque.enqueue(GitHook, owner.uname, name, sha, GitHook::ZERO, "refs/heads/#{branch}", 'commit', "user-#{user.id}", nil) - return true - end - def create_branch(new_ref, from_ref, user) - branch = repo.branches.detect{|b| b.name == from_ref} - return false if !branch || repo.branches.one?{|b| b.name == new_ref} - restore_branch new_ref, branch.commit.id, user + return false if new_ref.blank? || from_ref.blank? || !(from_commit = repo.commit(from_ref)) + status, out, err = repo.git.native(:branch, {:process_info => true}, new_ref, from_commit.id) + if status == 0 + Resque.enqueue(GitHook, owner.uname, name, from_commit.id, GitHook::ZERO, "refs/heads/#{new_ref}", 'commit', "user-#{user.id}", nil) + return true + end + return false + end def delete_branch(branch, user) From 4d4260fb7fac35e0226d7075cc13b81312ea8e2e Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Tue, 23 Jul 2013 17:41:05 +0400 Subject: [PATCH 08/10] #222: updated routes.rb --- app/views/projects/base/_repo_block.html.haml | 2 +- config/routes.rb | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/projects/base/_repo_block.html.haml b/app/views/projects/base/_repo_block.html.haml index 5bea6e279..e641613dc 100644 --- a/app/views/projects/base/_repo_block.html.haml +++ b/app/views/projects/base/_repo_block.html.haml @@ -42,7 +42,7 @@ %li{:class => ('selected' if act == :index && contr == :commits )} = link_to t('project_menu.commits'), commits_path(project, treeish) %li{:class => ('selected' if act == :branches && contr == :trees )} - = link_to t('project_menu.branches', :count => '{{singleton.project.branches_count}}'), branches_path(project, branch) + = link_to t('project_menu.branches', :count => '{{singleton.project.branches_count}}'), branch_path(project, branch) %li.tags{:class => ('selected' if act == :tags && contr == :trees )} = link_to t('project_menu.tags', :count => project.repo.tags.count), tags_path(project) .both diff --git a/config/routes.rb b/config/routes.rb index 942acad29..d61dc9afa 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -334,10 +334,10 @@ Rosa::Application.routes.draw do # Tags get '/tags' => "git/trees#tags", :as => :tags # Branches - get '/branches/:treeish' => "git/trees#branches", :as => :branches - delete '/branches/:treeish' => "git/trees#destroy", :as => :branches - put '/branches/:treeish' => "git/trees#restore_branch", :as => :branches - post '/branches' => "git/trees#create", :as => :branches + get '/branches/:treeish' => "git/trees#branches", :as => :branch + delete '/branches/:treeish' => "git/trees#destroy", :as => :branch + put '/branches/:treeish' => "git/trees#restore_branch", :as => :branch + post '/branches' => "git/trees#create", :as => :branchs # Commits get '/commits/:treeish(/*path)' => "git/commits#index", :as => :commits, :format => false get '/commit/:id(.:format)' => "git/commits#show", :as => :commit From 8f1d4e51185ffc033b74cba8cd51da15f5de1197 Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Tue, 23 Jul 2013 18:19:31 +0400 Subject: [PATCH 09/10] #222: fixed specs --- .../projects/git/git_trees_controller_spec.rb | 2 +- spec/models/project_spec.rb | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/controllers/projects/git/git_trees_controller_spec.rb b/spec/controllers/projects/git/git_trees_controller_spec.rb index eb73a0ed8..aca37adfd 100644 --- a/spec/controllers/projects/git/git_trees_controller_spec.rb +++ b/spec/controllers/projects/git/git_trees_controller_spec.rb @@ -116,7 +116,7 @@ describe Projects::Git::TreesController do end it 'should be able to perform restore_branch action' do - put :restore_branch, @params.merge(:treeish => 'conflicts') + put :restore_branch, @params.merge(:treeish => 'master-1', :sha => 'master') response.should be_success end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e61151ab9..502f2ef89 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -117,21 +117,21 @@ describe Project do end end - context '#restore_branch' do + context '#create_branch' do before do project.delete_branch(branch, user) end - xit 'ensures that returns true on success' do - project.restore_branch(branch.name, branch.commit.id).should be_true + it 'ensures that returns true on success' do + project.create_branch(branch.name, branch.commit.id, user).should be_true end - it 'ensures that branch has been restored' do - lambda { project.restore_branch(branch.name, branch.commit.id) }.should change{ project.repo.branches.count }.by(1) + it 'ensures that branch has been created' do + lambda { project.create_branch(branch.name, branch.commit.id, user) }.should change{ project.repo.branches.count }.by(1) end - xit 'ensures that returns false on restore wrong branch' do - project.restore_branch(branch.name, GitHook::ZERO).should be_false + it 'ensures that returns false on create wrong branch' do + project.create_branch(branch.name, GitHook::ZERO, user).should be_false end end From b10b990b474687ee435efc0eb1a8f28771fe4532 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Tue, 23 Jul 2013 20:30:34 +0600 Subject: [PATCH 10/10] Diff should be left alone by angular --- app/helpers/diff_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 2c522e519..6974ca1ad 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -37,7 +37,7 @@ module DiffHelper end prepare(args.merge({:filepath => filepath, :comments => comments, :in_discussion => in_discussion})) - res = "" + res = "
" res += "" res += renderer diff_display.data #diff_display.render(Git::Diff::InlineCallback.new comments, path) res += tr_line_comments(comments) if in_discussion