FEATURE: by default do not allow self approvals

FEATURE: by default do not allow self approvals

You can set code_review_allow_self_approval to true if you wish to
allow author of commit to approve commit

also updates icons to font awesome 5

resolves: Should we allow someone to approve their own commit?

From a27caf017cc9f2738ba5b9253d9a37d7d53bb989 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Tue, 27 Nov 2018 18:23:37 +1100
Subject: [PATCH] FEATURE: by default do not allow self approvals

You can set `code_review_allow_self_approval` to true if you wish to
allow author of commit to approve commit

also updates icons to font awesome 5

resolves: https://review.discourse.org/t/should-we-allow-someone-to-approve-their-own-commit/146

diff --git a/app/controllers/discourse_code_review/code_review_controller.rb b/app/controllers/discourse_code_review/code_review_controller.rb
index 5ca36b8..2db768e 100644
--- a/app/controllers/discourse_code_review/code_review_controller.rb
+++ b/app/controllers/discourse_code_review/code_review_controller.rb
@@ -79,6 +79,10 @@ module DiscourseCodeReview
 
       topic = Topic.find_by(id: params[:topic_id])
 
+      if !SiteSetting.code_review_allow_self_approval && topic.user_id == current_user.id
+        raise Discourse::InvalidAccess
+      end
+
       tags = topic.tags.pluck(:name)
 
       tags -= [
diff --git a/assets/javascripts/discourse/initializers/init-code-review.js.es6 b/assets/javascripts/discourse/initializers/init-code-review.js.es6
index fb86995..61dbb05 100644
--- a/assets/javascripts/discourse/initializers/init-code-review.js.es6
+++ b/assets/javascripts/discourse/initializers/init-code-review.js.es6
@@ -18,7 +18,7 @@ function actOnCommit(topic, action) {
 }
 
 function initialize(api) {
-  api.addPostSmallActionIcon("followup", "clock-o");
+  api.addPostSmallActionIcon("followup", "far-clock");
   api.addPostSmallActionIcon("approved", "thumbs-up");
 
   function allowUser() {
@@ -29,15 +29,30 @@ function initialize(api) {
     return currentUser.get("staff");
   }
 
+  function allowApprove(topic) {
+    const currentUser = api.getCurrentUser();
+    if (!currentUser) {
+      return false;
+    }
+
+    const allowSelfApprove = api.container.lookup("site-settings:main")
+      .code_review_allow_self_approval;
+
+    return allowSelfApprove || currentUser.get("id") !== topic.get("user_id");
+  }
+
   api
     .modifySelectKit("topic-footer-mobile-dropdown")
     .modifyContent((context, existingContent) => {
-      if (allowUser(context.get("currentUser"))) {
-        existingContent.push({
-          id: "approve",
-          icon: "thumbs-up",
-          name: I18n.t("code_review.approve.label")
-        });
+      const topic = context.get("topic");
+
+      if (allowUser()) {
+        if (allowApprove(topic))
+          existingContent.push({
+            id: "approve",
+            icon: "thumbs-up",
+            name: I18n.t("code_review.approve.label")
+          });
 
         existingContent.push({
           id: "followup",
@@ -61,6 +76,7 @@ function initialize(api) {
     {
       setupComponent(args) {
         this.set("topic", args.topic);
+        this.set("showApprove", allowApprove(args.topic));
       },
       shouldRender: function(args, component) {
         if (component.get("site.mobileView")) {
@@ -68,6 +84,7 @@ function initialize(api) {
         }
         return allowUser(args.topic);
       },
+
       actions: {
         followupCommit() {
           actOnCommit(this.get("topic"), "followup");
diff --git a/assets/javascripts/discourse/templates/connectors/topic-footer-main-buttons-before-create/approve.hbs b/assets/javascripts/discourse/templates/connectors/topic-footer-main-buttons-before-create/approve.hbs
index 9246ff6..89b8d4f 100644
--- a/assets/javascripts/discourse/templates/connectors/topic-footer-main-buttons-before-create/approve.hbs
+++ b/assets/javascripts/discourse/templates/connectors/topic-footer-main-buttons-before-create/approve.hbs
@@ -1,9 +1,11 @@
+{{#if showApprove}}
 {{d-button
   class="btn approve-commit-button"
   title="code_review.approve.title"
   label="code_review.approve.label"
   icon="thumbs-up"
   action="approveCommit"}}
+{{/if}}
 
 {{d-button
   class="btn follow-up-commit-button"
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 237a8c1..10cc65b 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -6,3 +6,4 @@ en:
     code_review_followup_tag: 'Tag to apply to follow up commits'
     code_review_approved_tag: 'Tag to apply to approved commits'
     code_review_github_webhook_secret: 'web hook secret string to use use for https://sitename/code-review/webhook'
+    code_review_allow_self_approval: 'Allow self approval of commits'
diff --git a/config/settings.yml b/config/settings.yml
index 274d7d9..9f548fa 100644
--- a/config/settings.yml
+++ b/config/settings.yml
@@ -5,3 +5,6 @@ plugins:
   code_review_approved_tag: "approved"
   code_review_followup_tag: "follow-up"
   code_review_github_webhook_secret: ""
+  code_review_allow_self_approval:
+    client: true
+    default: false
diff --git a/spec/requests/discourse_code_review/code_review_controller_spec.rb b/spec/requests/discourse_code_review/code_review_controller_spec.rb
index 49b83cb..102ffdb 100644
--- a/spec/requests/discourse_code_review/code_review_controller_spec.rb
+++ b/spec/requests/discourse_code_review/code_review_controller_spec.rb
@@ -5,7 +5,22 @@ describe DiscourseCodeReview::CodeReviewController do
     SiteSetting.code_review_enabled = true
   end
   context '.approve' do
-    it 'allows you to approve your own commit' do
+    it 'allows you to approve your own commit if enabled' do
+
+      SiteSetting.code_review_allow_self_approval = false
+
+      user = Fabricate(:admin)
+      commit = create_post(raw: "this is a fake commit", user: user, tags: ["hi", SiteSetting.code_review_pending_tag])
+
+      sign_in user
+
+      post '/code-review/approve.json', params: { topic_id: commit.topic_id }
+      expect(response.status).to eq(403)
+    end
+
+    it 'allows you to approve your own commit if enabled' do
+
+      SiteSetting.code_review_allow_self_approval = true
 
       another_commit = create_post(
         raw: "this is an old commit",
diff --git a/test/javascripts/acceptance/self-approve-test-desktop.js.es6 b/test/javascripts/acceptance/self-approve-test-desktop.js.es6
new file mode 100644
index 0000000..4fad8fc
--- /dev/null
+++ b/test/javascripts/acceptance/self-approve-test-desktop.js.es6
@@ -0,0 +1,20 @@
+import { replaceCurrentUser, acceptance } from "helpers/qunit-helpers";
+
+acceptance("review desktop", {
+  loggedIn: true,
+  mobileView: false
+});
+
+QUnit.test("shows approve button by default", async assert => {
+  await visit("/t/internationalization-localization/280");
+
+  assert.ok(count(".approve-commit-button") > 0);
+});
+
+QUnit.test("hides approve button if user is self", async assert => {
+  replaceCurrentUser({ id: 1 });
+
+  await visit("/t/this-is-a-test-topic/9/1");
+
+  assert.ok(count(".approve-commit-button") === 0);
+});
diff --git a/test/javascripts/acceptance/self-approve-test-mobile.js.es6 b/test/javascripts/acceptance/self-approve-test-mobile.js.es6
new file mode 100644
index 0000000..6a23832
--- /dev/null
+++ b/test/javascripts/acceptance/self-approve-test-mobile.js.es6
@@ -0,0 +1,35 @@
+import { replaceCurrentUser, acceptance } from "helpers/qunit-helpers";
+import { clearCallbacks } from "select-kit/mixins/plugin-api";
+
+acceptance("review mobile", {
+  loggedIn: true,
+  mobileView: true,
+  afterEach() {
+    clearCallbacks();
+  },
+  beforeEach() {
+    // we need to clean this up in core
+    // plugin api keeps being re-initialized
+    clearCallbacks();
+  }
+});
+
+QUnit.test("shows approve button by default", async assert => {
+  await visit("/t/internationalization-localization/280");
+
+  const menu = selectKit(".topic-footer-mobile-dropdown");
+  await menu.expand();
+
+  assert.ok(menu.rowByValue("approve").exists());
+});
+
+QUnit.test("hides approve button if user is self", async assert => {
+  replaceCurrentUser({ id: 1 });
+
+  await visit("/t/this-is-a-test-topic/9/1");
+
+  const menu = selectKit(".topic-footer-mobile-dropd

GitHub

1 Like

@jjaffeux this is pretty strange the way acceptance tests re-initialize plugins and then leak callbacks. This also tripped @nbianca

2 Likes