FEATURE: support consistent policy renewal dates

FEATURE: support consistent policy renewal dates

Previous to this change, policy renewal was solely conditional on policy creation date. This makes it very awkward to have policies that remind people of stuff every quarter cause the date is constantly floating

With this change you can use renew-start="01-01-2010" to determine the first date a renewal happens, after which repeat renewals will only be queue if N*renew-days + renew-start has passed.

This also is a major refactor of all policy internals, we no longer use a mountain of post custom fields, instead we have structured data in the post_policies table.

Yes, the migration is hairy.

Future plans include also moving the policy acceptance into a dedicated table so we can keep track of policy acceptance history.

diff --git a/app/models/post_policy.rb b/app/models/post_policy.rb
new file mode 100644
index 0000000..9ae679b
--- /dev/null
+++ b/app/models/post_policy.rb
@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class PostPolicy < ActiveRecord::Base
+  belongs_to :post
+  belongs_to :group
+end
diff --git a/assets/javascripts/initializers/extend-for-policy.js.es6 b/assets/javascripts/initializers/extend-for-policy.js.es6
index 65f10e4..cdfe4c8 100644
--- a/assets/javascripts/initializers/extend-for-policy.js.es6
+++ b/assets/javascripts/initializers/extend-for-policy.js.es6
@@ -12,6 +12,7 @@ const SETTINGS = [
   { name: "group", visible: true },
   { name: "version", visible: true, optional: true },
   { name: "renew", visible: true, optional: true },
+  { name: "renew-start", visible: true, optional: true },
   { name: "reminder", optional: true },
   { name: "accept", optional: true },
   { name: "revoke", optional: true }
diff --git a/assets/javascripts/lib/discourse-markdown/policy.js.es6 b/assets/javascripts/lib/discourse-markdown/policy.js.es6
index fb2ba57..f28e9bf 100644
--- a/assets/javascripts/lib/discourse-markdown/policy.js.es6
+++ b/assets/javascripts/lib/discourse-markdown/policy.js.es6
@@ -28,6 +28,10 @@ const rule = {
       token.attrs.push(["data-revoke", info.attrs.revoke]);
     }
 
+    if (info.attrs.start) {
+      token.attrs.push(["data-renew-start", info.attrs.start]);
+    }
+
     return true;
   }
 };
diff --git a/db/migrate/20190817010101_migrate_post_policy_table.rb b/db/migrate/20190817010101_migrate_post_policy_table.rb
new file mode 100644
index 0000000..c8a27ec
--- /dev/null
+++ b/db/migrate/20190817010101_migrate_post_policy_table.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class MigratePostPolicyTable < ActiveRecord::Migration[5.2]
+  def up
+    create_table(:post_policies) do |t|
+      t.bigint    :post_id, null: false, unique: true
+      t.timestamp :renew_start
+      t.integer   :renew_days
+      t.datetime  :next_renew_at
+      t.string    :reminder
+      t.datetime  :last_reminded_at
+      t.string    :version
+      t.integer   :group_id, null: false
+      t.timestamps
+    end
+  end
+
+  def down
+    drop_table :post_policies
+  end
+end
diff --git a/db/post_migrate/20190817010201_migrate_post_policy_data.rb b/db/post_migrate/20190817010201_migrate_post_policy_data.rb
new file mode 100644
index 0000000..d2d976d
--- /dev/null
+++ b/db/post_migrate/20190817010201_migrate_post_policy_data.rb
@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+class MigratePostPolicyData < ActiveRecord::Migration[5.2]
+  def up
+    execute(<<~SQL)
+      INSERT INTO post_policies(
+        post_id,
+        group_id,
+        version,
+        reminder,
+        last_reminded_at,
+        renew_days,
+        updated_at,
+        created_at
+      )
+      SELECT
+        f1.post_id,
+        (select id from groups where name ilike f1.value) group_id,
+        f2.value as version,
+        f3.value as reminder,
+        case when f4.value ~ '^[0-9]+$' then
+          TIMESTAMP 'epoch' + f4.value::integer * interval '1 second'
+          else null end
+        as last_reminded_at,
+        case when f5.value ~ '^[0-9]+$' then
+           f5.value::integer else null end
+        as renew_days,
+        greatest(f1.updated_at, f2.updated_at) as updated_at,
+        least(f1.updated_at, f2.updated_at) as created_at
+      FROM post_custom_fields f1
+      LEFT JOIN post_custom_fields f2 ON
+        f1.post_id = f2.post_id AND f2.name = 'PolicyVersion'
+      LEFT JOIN post_custom_fields f3 ON
+        f1.post_id = f3.post_id AND f3.name = 'PolicyReminder'
+      LEFT JOIN post_custom_fields f4 ON
+        f1.post_id = f4.post_id AND f4.name = 'LastRemindedAt'
+      LEFT JOIN post_custom_fields f5 ON
+        f1.post_id = f5.post_id AND f5.name = 'PolicyRenewDays'
+      WHERE f1.name = 'PolicyGroup'
+      AND  (select id from groups where name ilike f1.value) is not null
+      ON CONFLICT DO NOTHING
+    SQL
+
+    execute(<<~SQL)
+      DELETE FROM post_custom_fields
+      WHERE name in (
+        'PolicyGroup',
+        'PolicyVersion',
+        'PolicyReminder',
+        'PolicyRemindedAt',
+        'PolicyRenewDays'
+      )
+    SQL
+  end
+end
diff --git a/jobs/scheduled/check_policy.rb b/jobs/scheduled/check_policy.rb
index 94838e0..60221e3 100644
--- a/jobs/scheduled/check_policy.rb
+++ b/jobs/scheduled/check_policy.rb
@@ -8,37 +8,33 @@ module Jobs
 
     def execute(args = nil)
       sql = <<~SQL
-        SELECT p.id FROM post_custom_fields f
-        JOIN post_custom_fields f2 ON f2.post_id = f.post_id
-          AND f2.name = '#{::DiscoursePolicy::LastRemindedAt}'
-        JOIN posts p ON p.id = f.post_id
+        SELECT p.id FROM post_policies pp
+        JOIN posts p ON p.id = pp.post_id
         JOIN topics t ON t.id = p.topic_id
         WHERE t.deleted_at IS NULL
           AND p.deleted_at IS NULL
           AND t.archetype = 'regular'
-          AND f.name = '#{::DiscoursePolicy::PolicyReminder}'
           AND (
           (
-            f.value = 'weekly' AND
-            f2.value::integer < :weekly
+            reminder = 'weekly' AND
+            last_reminded_at < :weekly
           ) OR
           (
-            f.value = 'daily' AND
-            f2.value::integer < :daily
+            reminder = 'daily' AND
+            last_reminded_at < :daily
           ))
       SQL
 
       post_ids = DB.query_single(
         sql,
-        weekly: 1.week.ago.to_i,
-        daily: 1.day.ago.to_i
+        weekly: 1.week.ago,
+        daily: 1.day.ago
       )
 
       if post_ids.length > 0
         Post.where(id: post_ids).each do |post|
 
-          post.custom_fields[DiscoursePolicy::LastRemindedAt] = Time.now.to_i
-          post.save_custom_fields
+          post.post_policy.update(last_reminded_at: Time.zone.now)
 
           missing_users(post).each do |user|
             user.notifications.create!(
@@ -51,30 +47,39 @@ 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
+        next_renew = policy.renew_start
+        if policy.renew_days < 1
+          Rails.logger.warn("Invalid policy on post #{policy.post_id}")
+        else
+          while next_renew < Time.zone.now
+            next_renew += policy.renew_days.days
+          end
+        end
+        policy.update(next_renew_at: next_renew)
+      end
+
       sql = <<~SQL
       DELETE FROM post_custom_fields p
-      USING post_custom_fields p2
-      WHERE p.post_id = p2.post_id AND
+      USING post_policies pp
+      WHERE p.post_id = pp.post_id AND
+        pp.renew_start IS NULL AND
         p.name = :accepted_by AND
-        p2.name = :renew_days AND
-        p2.created_at < :now::timestamp - ( INTERVAL '1 day' *  p2.value::int )
+        pp.created_at < :now::timestamp - ( INTERVAL '1 day' *  pp.renew_days )
       SQL
 
       DB.exec(
         sql,
         accepted_by: DiscoursePolicy::AcceptedBy,
-        renew_days: DiscoursePolicy::PolicyRenewDays,
         now: Time.zone.now
       )
 
     end
 
     def missing_users(post)
-      if !group_name = post.custom_fields[DiscoursePolicy::PolicyGroup]
-        return []
-      end
 
-      group = Group.find_by(name: group_name)
+      group = post.post_policy.group
 
       if !group
         return []
diff --git a/plugin.rb b/plugin.rb
index ab79eab..1a80a97 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -15,15 +15,10 @@ enabled_site_setting :policy_enabled
 PLUGIN_NAME ||= "discourse_policy".freeze
 
 after_initialize do
-
   module ::DiscoursePolicy
 
     AcceptedBy = "PolicyAcceptedBy"
-    PolicyGroup = "PolicyGroup"
-    PolicyVersion = "PolicyVersion"
-    PolicyReminder = "PolicyReminder"
-    LastRemindedAt = "LastRemindedAt"
-    PolicyRenewDays = "PolicyRenewDays"
+    HasPolicy = "HasPolicy"
 
     class Engine < ::Rails::Engine

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

GitHub sha: 7852c7ba

2 Likes

All these fields are NULLable?

Looks like we should add an index on next_renew_at

yeah, some policies never renew, have not reminders or version

Yeah possibly, there are so few rows though in this table, I imagine it would be ultra rare to have anything more than a few 100 policies on a site