FEATURE: history for policy plugin (#8)

FEATURE: history for policy plugin (#8)

Instead of using CustomField, the policy is using PolicyUsers to keep track of acceptance history

diff --git a/app/models/policy_user.rb b/app/models/policy_user.rb
new file mode 100644
index 0000000..d4e40c3
--- /dev/null
+++ b/app/models/policy_user.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+class PolicyUser < ActiveRecord::Base
+  belongs_to :post_policy
+  belongs_to :user
+
+  scope :accepted, -> { where.not(accepted_at: nil).where(revoked_at: nil, expired_at: nil) }
+  scope :with_version, ->(version) { where(version: version) }
+
+  def self.add!(user, post_policy)
+    self.create!(
+      post_policy_id: post_policy.id,
+      user_id: user.id,
+      accepted_at: Time.zone.now,
+      version: post_policy.version
+    )
+  end
+
+  def self.remove!(user, post_policy)
+    post_policy
+      .policy_users
+      .accepted
+      .with_version(post_policy.version)
+      .where(user: user)
+      .update_all(revoked_at: Time.zone.now)
+  end
+end
diff --git a/app/models/post_policy.rb b/app/models/post_policy.rb
index 9ae679b..f6462e6 100644
--- a/app/models/post_policy.rb
+++ b/app/models/post_policy.rb
@@ -3,4 +3,38 @@
 class PostPolicy < ActiveRecord::Base
   belongs_to :post
   belongs_to :group
+  has_many :policy_users
+
+  def accepted_by
+    return [] if !policy_group
+    policy_users
+      .accepted
+      .with_version(version)
+      .where(user_id: policy_group_users.map(&:id))
+      .includes(:user)
+      .map(&:user)
+  end
+
+  def not_accepted_by
+    return [] if !policy_group
+    policy_group_users - accepted_by
+  end
+
+  private
+
+  def policy_group_users
+    @policy_group_users ||= User.joins(:group_users)
+      .where('group_users.group_id = ?', policy_group.id)
+      .select(:id, :username, :uploaded_avatar_id).to_a
+  end
+
+  def policy_group
+    return @policy_group == :nil ? nil : @policy_group if @policy_group
+    @policy_group = :nil
+
+    @policy_group = Group
+      .where('user_count < ?', SiteSetting.policy_max_group_size)
+      .where('id in (SELECT group_id FROM post_policies WHERE post_id = ?)', post.id)
+      .first || :nil
+  end
 end
diff --git a/db/migrate/20191013212445_migrate_policy_users_table.rb b/db/migrate/20191013212445_migrate_policy_users_table.rb
new file mode 100644
index 0000000..dd7d33c
--- /dev/null
+++ b/db/migrate/20191013212445_migrate_policy_users_table.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class MigratePolicyUsersTable < ActiveRecord::Migration[6.0]
+  def change
+    create_table :policy_users do |t|
+      t.integer :post_policy_id, null: false
+      t.integer :user_id, null: false
+      t.datetime :accepted_at
+      t.datetime :revoked_at
+      t.datetime :expired_at
+      t.string :version
+      t.timestamps null: false
+    end
+
+    add_index :policy_users, %i[post_policy_id user_id]
+  end
+end
diff --git a/db/post_migrate/20191014224419_migrate_custom_field_to_policy_users.rb b/db/post_migrate/20191014224419_migrate_custom_field_to_policy_users.rb
new file mode 100644
index 0000000..cfea11d
--- /dev/null
+++ b/db/post_migrate/20191014224419_migrate_custom_field_to_policy_users.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class MigrateCustomFieldToPolicyUsers < ActiveRecord::Migration[6.0]
+  def up
+    execute(<<~SQL)
+    INSERT INTO policy_users(post_policy_id, user_id, version, accepted_at, created_at, updated_at)
+    SELECT post_policies.id, post_custom_fields.value::INTEGER, post_policies.version, post_custom_fields.created_at, post_custom_fields.created_at, post_custom_fields.updated_at
+    FROM post_custom_fields
+    INNER JOIN post_policies ON post_policies.post_id = post_custom_fields.post_id
+    WHERE post_custom_fields.name = 'PolicyAcceptedBy'
+    SQL
+
+    execute(<<~SQL)
+    DELETE FROM post_custom_fields WHERE name = 'PolicyAcceptedBy'
+    SQL
+  end
+end
diff --git a/jobs/scheduled/check_policy.rb b/jobs/scheduled/check_policy.rb
index 4c1dab7..b63fb94 100644
--- a/jobs/scheduled/check_policy.rb
+++ b/jobs/scheduled/check_policy.rb
@@ -32,7 +32,7 @@ module Jobs
       )
 
       if post_ids.length > 0
-        Post.where(id: post_ids).each do |post|
+        Post.where(id: post_ids).find_each do |post|
 
           post.post_policy.update(last_reminded_at: Time.zone.now)
 
@@ -47,8 +47,8 @@ module Jobs
         end
       end
 
-      PostPolicy.where('next_renew_at < ?', Time.zone.now).each do |policy|
-        PostCustomField.where(name: DiscoursePolicy::AcceptedBy, post_id: policy.post_id).delete_all
+      PostPolicy.where('next_renew_at < ?', Time.zone.now).find_each do |policy|
+        policy.policy_users.accepted.update_all(expired_at: Time.zone.now)
         next_renew = policy.renew_start
         if policy.renew_days < 1
           Rails.logger.warn("Invalid policy on post #{policy.post_id}")
@@ -61,34 +61,26 @@ module Jobs
       end
 
       sql = <<~SQL
-      DELETE FROM post_custom_fields f
-      USING post_policies pp
-      WHERE f.post_id = pp.post_id AND
-        pp.renew_start IS NULL AND
-        f.name = :accepted_by AND
-        f.created_at < :now::timestamp - ( INTERVAL '1 day' *  pp.renew_days )
+      UPDATE policy_users
+      SET expired_at = :now
+      FROM policy_users pu
+      INNER JOIN post_policies pp ON pp.id = pu.post_policy_id
+      WHERE pp.renew_start IS NULL AND
+      pp.renew_days IS NOT NULL AND
+      pu.accepted_at IS NOT NULL AND
+      pu.expired_at IS NULL AND
+      pu.revoked_at IS NULL AND
+      pu.accepted_at < :now::timestamp - ( INTERVAL '1 day' *  pp.renew_days )
       SQL
 
       DB.exec(
         sql,
-        accepted_by: DiscoursePolicy::AcceptedBy,
         now: Time.zone.now
       )
-
     end
 
     def missing_users(post)
-
-      group = post.post_policy.group
-
-      if !group
-        return []
-      end
-
-      User.joins(:group_users)
-        .where('group_users.group_id = ?', group.id)
-        .where('users.id NOT IN (?)', post.custom_fields[DiscoursePolicy::AcceptedBy] || [-1])
+      post.post_policy.not_accepted_by
     end
-
   end
 end
diff --git a/plugin.rb b/plugin.rb
index 5d87b30..e499bf0 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -17,7 +17,6 @@ PLUGIN_NAME ||= "discourse_policy".freeze
 after_initialize do
   module ::DiscoursePolicy
 
-    AcceptedBy = "PolicyAcceptedBy"
     HasPolicy = "HasPolicy"
 
     class Engine < ::Rails::Engine
@@ -29,6 +28,7 @@ after_initialize do
   [
     "../jobs/scheduled/check_policy.rb",
     "../app/models/post_policy",
+    "../app/models/policy_user",
   ].each { |path| require File.expand_path(path, __FILE__) }
 
   require 'post'
@@ -36,8 +36,6 @@ after_initialize do
     has_one :post_policy, dependent: :destroy
   end
 
-  Post.register_custom_field_type DiscoursePolicy::AcceptedBy, [:integer]
-
   require_dependency "application_controller"
   class DiscoursePolicy::PolicyController < ::ApplicationController
     requires_plugin PLUGIN_NAME
@@ -84,18 +82,9 @@ after_initialize do
       end
 
       if type == :add
-        PostCustomField.create(
-          post_id: post.id,
-          name: DiscoursePolicy::AcceptedBy,
-          value: current_user.id
-        )
+        PolicyUser.add!(current_user, post.post_policy)
       else
-        # API needs love here...
-        PostCustomField.where(
-          post_id: post.id,
-          name: DiscoursePolicy::AcceptedBy,
-          value: current_user.id
-        ).delete_all
+        PolicyUser.remove!(current_user, post.post_policy)
       end
 
       post.publish_change_to_clients!(:policy_change)
@@ -156,11 +145,7 @@ after_initialize do
 
         if version = policy["data-version"]
           old_version = post_policy.version || "1"
-          if version != old_version
-            post.custom_fields[DiscoursePolicy::AcceptedBy] = []
-            post_policy.version = version
-            post.save_custom_fields
-          end
+          post_policy.version = version if version != old_version
         end
 
         if reminder = policy["data-reminder"]
@@ -188,54 +173,26 @@ after_initialize do
   # on(:post_created) do |post|
   # end
 

[... diff too long, it was truncated ...]

GitHub sha: ec1d4d9f

1 Like
return @policy_group == :nil ? nil : @policy_group if @policy_group 
@policy_group = :nil

I see what this code is doing here - it wants to memoize nil which is not normally memoizable with the tranditional ||= syntax.

However, you end up storing two different kinds of things in the same variable and the code has to check. It’s obviously OK in this case because you added an accessor method. However, in the future a programmer might miss that method and use @policy_group directly by accident after inspecting its value and not realizing that it could also be a symbol :nil which sets them up for failure later.

I would recommend using defined? instead:

3 Likes

thank you @eviltrout for that review, you are right that this approach is much nicer :slight_smile:

Fix is here FIX: better memoize of policy_group by lis2 · Pull Request #9 · discourse/discourse-policy · GitHub

Note, I introduced the :nil pattern (possibly here and in Discourse core in a few places). I am fine switching this to use defined? it certainly simplifies reasoning about the code.

2 Likes

I would very much like to consider that technical debt and upgrade it over time. We needn’t stop everything to do it though :slight_smile:

1 Like