diff --git a/CHANGELOG.md b/CHANGELOG.md index abae1d60b..d1670e59f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Team and namespace descriptions can be written using Markdown. See pull requests: [#546](https://github.com/SUSE/Portus/pull/546) and [#531](https://github.com/SUSE/Portus/pull/531). +- Team members can comment on repositories. See pull request: [#538](https://github.com/SUSE/Portus/pull/583) ## 2.0.0 diff --git a/app/assets/javascripts/comments.coffee b/app/assets/javascripts/comments.coffee new file mode 100644 index 000000000..4905031e5 --- /dev/null +++ b/app/assets/javascripts/comments.coffee @@ -0,0 +1,8 @@ +$(document).on "page:change", -> + + # Shows and hides the comment form + $('#write_comment_repository_btn').unbind('click').on 'click', (event) -> + $('#write_comment_form').toggle 400, "swing", -> + if $('#write_comment_form').is(':visible') + $('#comment_body').focus() + layout_resizer() diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index c0d560d6f..253c6f7d4 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -27,4 +27,5 @@ @import 'tables'; @import 'forms'; @import 'types'; +@import 'comments'; @import 'responsive-utilities'; diff --git a/app/assets/stylesheets/comments.scss b/app/assets/stylesheets/comments.scss new file mode 100644 index 000000000..55b10b049 --- /dev/null +++ b/app/assets/stylesheets/comments.scss @@ -0,0 +1,15 @@ +#write_comment_repository_btn, .destroy_comments_btn { + padding-top: 0px; + padding-bottom: 0px; + border: 0px; +} + +.comment-thumbnail { + padding: 10px 0px; + img { + border-radius: 100%; + border: 4px solid $gray-light; + width: 40px; + } +} + diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb new file mode 100644 index 000000000..87b613f05 --- /dev/null +++ b/app/controllers/comments_controller.rb @@ -0,0 +1,33 @@ +class CommentsController < ApplicationController + before_action :set_repository + respond_to :js, :html + + # POST /repositories/1/comments + # POST /repositories/1/comments.json + def create + @comment = @repository.comments.new(params.require(:comment).permit(:body)) + @comment.author = current_user + authorize @comment + + if @comment.save + respond_with(@comment) + else + respond_with @comment.errors, status: :unprocessable_entity + end + end + + # DELETE /repositories/1/comments/1 + # DELETE /repositories/1/comments/1.json + def destroy + @comment = @repository.comments.find(params[:id]) + authorize @comment + @comment.destroy + respond_with @comment + end + + private + + def set_repository + @repository = Repository.find(params[:repository_id]) + end +end diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index 30cee74f7..dfc202386 100644 --- a/app/controllers/repositories_controller.rb +++ b/app/controllers/repositories_controller.rb @@ -13,6 +13,7 @@ def index def show authorize @repository @tags = @repository.tags.order("created_at DESC") + @repository_comments = @repository.comments.all respond_with(@repository) end diff --git a/app/models/comment.rb b/app/models/comment.rb new file mode 100644 index 000000000..f359739be --- /dev/null +++ b/app/models/comment.rb @@ -0,0 +1,12 @@ +class Comment < ActiveRecord::Base + include PublicActivity::Common + belongs_to :repository + belongs_to :author, class_name: "User", foreign_key: "user_id" + + validates :body, presence: true + + # Returns true if the user is the author of the comment + def author?(user) + user_id == user.id + end +end diff --git a/app/models/repository.rb b/app/models/repository.rb index 4c562685e..476bf80d3 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -5,6 +5,7 @@ class Repository < ActiveRecord::Base belongs_to :namespace has_many :tags, dependent: :delete_all has_many :stars, dependent: :delete_all + has_many :comments, dependent: :delete_all # We don't validate the format because we get that from the registry, and # it's guaranteed to be well-formatted there. diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb new file mode 100644 index 000000000..90a3203d3 --- /dev/null +++ b/app/policies/comment_policy.rb @@ -0,0 +1,22 @@ +class CommentPolicy + attr_reader :user, :comment + + def initialize(user, comment) + @user = user + @comment = comment + end + + # Allows the admin to write comments + # Allows all members of a team to comment on their repos + # Allows all users to comment on repos under a public namespace + def create? + @user.admin? || + @comment.repository.namespace.public? || + @comment.repository.namespace.team.team_users.where(user_id: @user.id).any? + end + + # Allows only the admin and author to delete comments + def destroy? + @user.admin? || @comment.author?(@user) + end +end diff --git a/app/views/comments/_comment.html.slim b/app/views/comments/_comment.html.slim new file mode 100644 index 000000000..d7c95f1e7 --- /dev/null +++ b/app/views/comments/_comment.html.slim @@ -0,0 +1,29 @@ +.row id="comment_#{comment.id}" + .col-md-1.comment-thumbnail + .pull-right + .user-image + = user_image_tag(comment.author.email) + .col-md-11 + .panel.panel-default + .panel-heading + h5 + strong + = comment.author.username + ' commented + = activity_time_tag comment.updated_at + ' ago + .pull-right + button.btn.btn-link.btn-x.btn-edit-role.destroy_comments_btn[ + data-placement="left" + data-toggle="popover" + data-title="Please confirm" + data-content='

Are you sure you want to remove this \ + comment?

No Yes' + data-html="true" + role="button"] + if.fa.fa-trash + | Delete comment + .panel-body + = markdown(comment.body) diff --git a/app/views/comments/create.js.erb b/app/views/comments/create.js.erb new file mode 100644 index 000000000..cceb29fa8 --- /dev/null +++ b/app/views/comments/create.js.erb @@ -0,0 +1,15 @@ +<% if @comment.errors.any? %> + $('#alert p').html("<%= escape_javascript(@comment.errors.full_messages.join('
')) %>"); +<% else %> + <% if @repository.comments.count == 1 %> + $(".comment_string").text(" Comment"); + $(".container-full").text("") + <% elsif @repository.comments.count == 2 %> + $(".comment_string").text(" Comments"); + <% end %> + $(".number_of_comments").text("<%= escape_javascript(@repository.comments.count.to_s) %>"); + $("<%= escape_javascript(render @comment) %>").appendTo(".container-full"); + $('#alert p').html("New comment posted"); + $('#write_comment_form').fadeOut(); +<% end %> +$('#alert').fadeIn(); diff --git a/app/views/comments/destroy.js.erb b/app/views/comments/destroy.js.erb new file mode 100644 index 000000000..e70a8b5b2 --- /dev/null +++ b/app/views/comments/destroy.js.erb @@ -0,0 +1,14 @@ +<% if @comment.errors.any? %> + $('#alert p').html("<%= escape_javascript(@comment.errors.full_messages.join('
')) %>"); +<% else %> + <% if @repository.comments.count == 0 %> + $(".comment_string").text(" Comments"); + $(".container-full").text("Nobody has left a comment yet."); + <% elsif @repository.comments.count == 1 %> + $(".comment_string").text(" Comment"); + <% end %> + $("#comment_<%= escape_javascript(@comment.id.to_s) %>").remove(); + $(".number_of_comments").text("<%= escape_javascript(@repository.comments.count.to_s) %>"); + $('#alert p').html("Comment successfully deleted"); +<% end %> +$('#alert').fadeIn(); diff --git a/app/views/repositories/show.html.slim b/app/views/repositories/show.html.slim index 0d4eab659..f7cb01fc0 100644 --- a/app/views/repositories/show.html.slim +++ b/app/views/repositories/show.html.slim @@ -13,7 +13,7 @@ = link_to toggle_star_repository_path(@repository), method: :post, title: 'Star repository', class: 'btn btn-small btn-default btn-xs', remote: true, id: 'toggle_star' do i.fa.fa-star-o span#star-counter.btn.btn-primary.btn-xs - = @repository.stars.count + = @repository.stars.count .panel-body .table-responsive @@ -34,3 +34,35 @@ = tag.name td= tag.author.username td= tag.created_at + +#write_comment_form.collapse + = form_for [@repository, @repository.comments.build], remote: true, html: {id: 'new-comment-form', class: 'form-horizontal', role: 'form'} do |f| + .form-group + = f.label :comment, {class: 'control-label col-md-2'} + .col-md-7 + = f.text_area(:body, class: 'form-control', required: false, placeholder: "Please write a comment.") + .form-group + .col-md-offset-2.col-md-7 + = f.submit('Add', class: 'btn btn-primary') + +.panel.panel-default + .panel-heading.clearfix + h5 + .pull-right + a#write_comment_repository_btn.btn.btn-xs.btn-link.js-toggle-button role="button" + i.fa.fa-comment + | Write a comment + span class="number_of_comments" + = @repository.comments.count + span class="comment_string" + - if @repository.comments.count == 1 + | Comment + - else + | Comments + #comments.panel-body + .container-full + - if @repository_comments.empty? + ' Nobody has left a comment yet. + - else + - @repository_comments.each do |comment| + = render(comment) diff --git a/config/routes.rb b/config/routes.rb index ec69c113f..870887ea2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -13,6 +13,7 @@ resources :repositories, only: [:index, :show] do post :toggle_star, on: :member + resources :comments, only: [:create, :destroy] end devise_for :users, controllers: { registrations: "auth/registrations", diff --git a/db/migrate/20151113162311_create_comments.rb b/db/migrate/20151113162311_create_comments.rb new file mode 100644 index 000000000..25fcb30e4 --- /dev/null +++ b/db/migrate/20151113162311_create_comments.rb @@ -0,0 +1,11 @@ +class CreateComments < ActiveRecord::Migration + def change + create_table :comments do |t| + t.string :title + t.text :body + t.references :repository, index: true, foreign_key: true + + t.timestamps null: false + end + end +end diff --git a/db/migrate/20151113162513_add_author_to_comment.rb b/db/migrate/20151113162513_add_author_to_comment.rb new file mode 100644 index 000000000..448afbb1e --- /dev/null +++ b/db/migrate/20151113162513_add_author_to_comment.rb @@ -0,0 +1,5 @@ +class AddAuthorToComment < ActiveRecord::Migration + def change + add_belongs_to :comments, :user, index: true + end +end diff --git a/db/migrate/20151124150353_removetitle.rb b/db/migrate/20151124150353_removetitle.rb new file mode 100644 index 000000000..7505751c0 --- /dev/null +++ b/db/migrate/20151124150353_removetitle.rb @@ -0,0 +1,5 @@ +class Removetitle < ActiveRecord::Migration + def change + remove_column :comments, :title + end +end diff --git a/db/schema.rb b/db/schema.rb index 502a5e29a..d8de352c9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20151112003047) do +ActiveRecord::Schema.define(version: 20151124150353) do create_table "activities", force: :cascade do |t| t.integer "trackable_id", limit: 4 @@ -31,6 +31,17 @@ add_index "activities", ["recipient_id", "recipient_type"], name: "index_activities_on_recipient_id_and_recipient_type", using: :btree add_index "activities", ["trackable_id", "trackable_type"], name: "index_activities_on_trackable_id_and_trackable_type", using: :btree + create_table "comments", force: :cascade do |t| + t.text "body", limit: 65535 + t.integer "repository_id", limit: 4 + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.integer "user_id", limit: 4 + end + + add_index "comments", ["repository_id"], name: "index_comments_on_repository_id", using: :btree + add_index "comments", ["user_id"], name: "index_comments_on_user_id", using: :btree + create_table "crono_jobs", force: :cascade do |t| t.string "job_id", limit: 255, null: false t.text "log", limit: 65535 @@ -150,6 +161,7 @@ add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree add_index "users", ["username"], name: "index_users_on_username", unique: true, using: :btree + add_foreign_key "comments", "repositories" add_foreign_key "stars", "repositories" add_foreign_key "stars", "users" end diff --git a/spec/api/v2/token_spec.rb b/spec/api/v2/token_spec.rb index c50a55962..a63ba4975 100644 --- a/spec/api/v2/token_spec.rb +++ b/spec/api/v2/token_spec.rb @@ -199,7 +199,7 @@ valid_auth_header end - it "allows to pull an image in which this user is just a viewer" do + it "allows to pull an image in which this user is just a viewer" do # Quick way to force a "viewer" policy. allow_any_instance_of(NamespacePolicy).to receive(:push?).and_return(false) allow_any_instance_of(NamespacePolicy).to receive(:pull?).and_return(true) diff --git a/spec/controllers/comments_controller_spec.rb b/spec/controllers/comments_controller_spec.rb new file mode 100644 index 000000000..4df436aec --- /dev/null +++ b/spec/controllers/comments_controller_spec.rb @@ -0,0 +1,90 @@ +require "rails_helper" + +describe CommentsController, type: :controller do + let(:admin) { create(:admin) } + let(:owner) { create(:user) } + let(:user) { create(:user) } + let!(:team) { create(:team, owners: [owner]) } + let!(:public_namespace) { create(:namespace, public: 1, team: team) } + let!(:visible_repository) { create(:repository, namespace: public_namespace) } + let!(:private_namespace) { create(:namespace, public: 0, team: team) } + let!(:invisible_repository) { create(:repository, namespace: private_namespace) } + let(:comment) { create(:comment, author: owner) } + let!(:commented_repository) do + create(:repository, comments: [comment], namespace: private_namespace) + end + + let(:valid_attributes) do + { body: "short test comment" } + end + + let(:invalid_attributes) do + { foo: "not valid" } + end + + describe "PUT #create" do + context "with valid params" do + it "creates a new comment" do + sign_in owner + expect do + repository_id = invisible_repository.id + post :create, repository_id: repository_id, comment: valid_attributes, format: "js" + expect(response.status).to eq 200 + end.to change(Comment, :count).by(1) + expect(assigns(:comment).author.id).to eq owner.id + end + + it "does allow everyone to write comments for a repository under a public namespace" do + sign_in user + post :create, repository_id: visible_repository.id, comment: valid_attributes, format: :js + expect(response.status).to eq 200 + end + + it "does allow the admin to write comments for every repository" do + sign_in admin + post :create, repository_id: invisible_repository.id, comment: valid_attributes, format: :js + expect(response.status).to eq 200 + end + + it "does not allow a user who has no access to the repository to write comments" do + sign_in user + post :create, repository_id: invisible_repository.id, comment: valid_attributes, format: :js + expect(response.status).to eq 401 + end + end + + context "with invalid params" do + it "does not allow invalid parameters" do + sign_in owner + repository_id = invisible_repository.id + post :create, repository_id: repository_id, comment: invalid_attributes, format: :js + expect(assigns(:comment)).to be_a_new(Comment) + expect(response.status).to eq 422 + end + end + end + + describe "DELETE #destroy" do + it "deletes a comment" do + sign_in owner + expect do + delete :destroy, repository_id: commented_repository.id, id: comment.id, format: :js + end.to change(Comment, :count).by(-1) + end + + it "does allow to delete a comment if the user is admin" do + sign_in admin + expect do + delete :destroy, repository_id: commented_repository.id, id: comment.id, format: :js + end.to change(Comment, :count).by(-1) + end + + it "does not allow to delete the comment if the user is not the author" do + sign_in user + expect do + delete :destroy, repository_id: commented_repository.id, id: comment.id, format: :js + end.to change(Comment, :count).by(0) + expect(response.status).to eq 401 + end + end +end diff --git a/spec/controllers/repositories_controller_spec.rb b/spec/controllers/repositories_controller_spec.rb index 8fb5b6bfb..13257352f 100644 --- a/spec/controllers/repositories_controller_spec.rb +++ b/spec/controllers/repositories_controller_spec.rb @@ -28,7 +28,6 @@ get :show, { id: visible_repository.to_param }, valid_session expect(assigns(:repository)).to eq(visible_repository) end - end describe "POST #toggle_star" do diff --git a/spec/factories/comments.rb b/spec/factories/comments.rb new file mode 100644 index 000000000..662e90595 --- /dev/null +++ b/spec/factories/comments.rb @@ -0,0 +1,9 @@ +FactoryGirl.define do + factory :comment do + sequence :body do |b| + "a short comment #{b}" + end + + association :author, factory: :user + end +end diff --git a/spec/jobs/catalog_job_spec.rb b/spec/jobs/catalog_job_spec.rb index 914ecf921..08085ba09 100644 --- a/spec/jobs/catalog_job_spec.rb +++ b/spec/jobs/catalog_job_spec.rb @@ -52,7 +52,7 @@ def update_registry!(catalog) end it "performs the job as expected" do - registry = create(:registry, "hostname" => "registry.test.lan") + registry = create(:registry, "hostname" => "registry.test.lan") VCR.use_cassette("registry/get_registry_catalog", record: :none) do job = CatalogJobMock.new diff --git a/spec/lib/portus/config_spec.rb b/spec/lib/portus/config_spec.rb index 3c9979d4b..cf8b0a34c 100644 --- a/spec/lib/portus/config_spec.rb +++ b/spec/lib/portus/config_spec.rb @@ -47,7 +47,7 @@ def strict_merge_with_env(cfg, local, prefix = "portus") end it "raises an error when the local file is badly formatted" do - bad = get_config("config.yml", "bad.yml") + bad = get_config("config.yml", "bad.yml") expect { bad.fetch }.to raise_error(StandardError, "Wrong format for the config-local file!") end diff --git a/spec/lib/portus/jwt_token_spec.rb b/spec/lib/portus/jwt_token_spec.rb index b0dd65270..ed491981c 100644 --- a/spec/lib/portus/jwt_token_spec.rb +++ b/spec/lib/portus/jwt_token_spec.rb @@ -4,7 +4,7 @@ Portus::JwtToken.class_eval { attr_reader :account, :service } describe Portus::JwtToken do - let(:mock) { Portus::JwtToken.new("", "", nil) } + let(:mock) { Portus::JwtToken.new("", "", nil) } let(:registry) { create(:registry) } let(:scope) { Namespace::AuthScope.new(registry, "repository:samalba/my-app:push") } diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb new file mode 100644 index 000000000..0047a69e2 --- /dev/null +++ b/spec/models/comment_spec.rb @@ -0,0 +1,10 @@ +require "rails_helper" + +describe Comment do + it { should belong_to(:repository) } + it { should belong_to(:author) } + + it "has a valid factory" do + expect { Factory.build(:comment).to be_valid } + end +end diff --git a/spec/policies/repository_policy_spec.rb b/spec/policies/repository_policy_spec.rb index 7dab133e1..7c75ac475 100644 --- a/spec/policies/repository_policy_spec.rb +++ b/spec/policies/repository_policy_spec.rb @@ -73,7 +73,7 @@ let!(:namespace) { create(:namespace, team: team, name: "mssola") } let!(:repository) { create(:repository, namespace: namespace, name: "repository") } - it "finds the same repository regardless to how it has been written" do + it "finds the same repository regardless to how it has been written" do %w(repository rep epo).each do |name| repo = Pundit.policy_scope(user, Repository).search(name) expect(repo.name).to eql "Repository"