Merge pull request #4373 from dodona-edu/chore/tweak-notifications

Limit the number of notifications requests

Gemfile

@@ -140,6 +140,9 @@ gem 'ddtrace', '~> 1.8.0'
 # Make sure filesystem changes only happen at the end of a transaction
 gem 'after_commit_everywhere', '~> 1.3.0'
 
+# More advanced counter_cache that allows conditions
+gem 'counter_culture', '~> 3.2'
+
 group :development, :test do
   # Use mocha for stubbing and mocking
   gem 'mocha', '~> 2.0.2'

Gemfile.lock

@@ -122,6 +122,9 @@ GEM
     codecov (0.6.0)
       simplecov (>= 0.15, < 0.22)
     concurrent-ruby (1.2.0)
+    counter_culture (3.3.0)
+      activerecord (>= 4.2)
+      activesupport (>= 4.2)
     crack (0.4.5)
       rexml
     crass (1.0.6)
@@ -508,6 +511,7 @@ DEPENDENCIES
   capistrano3-delayed-job (~> 1.7.6)
   capybara (~> 3.38.0)
   codecov (~> 0.6.0)
+  counter_culture (~> 3.2)
   cssbundling-rails (~> 1.1.2)
   dalli (~> 3.2.3)
   ddtrace (~> 1.8.0)

app/assets/javascripts/auto_reload.ts

@@ -67,12 +67,14 @@ export class InactiveTimeout {
      * @param {HTMLElement} element The element for detecting user interaction.
      * @param {number} delay The delay in ms.
      * @param {function()} callback Function to call when the timout hits.
+     * @param {inactiveIncrease} number How much ms the delay is increased if the page is inactive.
      */
-    constructor(element: HTMLElement, delay: number, callback: () => void) {
+    constructor(element: HTMLElement, delay: number, callback: () => void, inactiveIncrease = 1000) {
         this.interactionElement = element;
         this.callback = callback;
         this.initialDelay = delay;
         this.delay = delay;
+        this.inactiveIncrease = inactiveIncrease;
 
         this.listener = () => {
             requestAnimationFrame(() => {

app/assets/javascripts/code_listing/question_annotation.ts

@@ -6,6 +6,7 @@ import {
     UserAnnotationEditor, UserAnnotationFormData,
     UserAnnotationPermissionData
 } from "code_listing/user_annotation";
+import { Notification } from "notification";
 
 interface QuestionAnnotationPermissionData extends UserAnnotationPermissionData {
     transition: {[state in QuestionState]: boolean};
@@ -154,6 +155,9 @@ export async function createUserAnnotation(formData: UserAnnotationFormData,
     const data = await response.json();
 
     if (response.ok) {
+        if (mode === "question") {
+            Notification.startNotificationRefresh();
+        }
         return annotationFromData(data, editFn);
     }
     throw new Error();

app/assets/javascripts/notification.ts

@@ -1,5 +1,6 @@
 import { FaviconManager } from "favicon";
 import { fetch } from "util.js";
+import { InactiveTimeout } from "auto_reload";
 
 /**
  * Model for a notification in the navbar. It adds three listeners to the notification view:
@@ -87,4 +88,18 @@ export class Notification {
         const response = await fetch("/notifications.js?per_page=5");
         eval(await response.text());
     }
+
+    static notificationRefreshTimeout: InactiveTimeout;
+    static startNotificationRefresh(): void {
+        if (Notification.notificationRefreshTimeout == undefined) {
+            Notification.notificationRefreshTimeout = new InactiveTimeout(
+                document.getElementById("page-wrapper"),
+                60 * 1000,
+                Notification.checkNotifications,
+                60 * 1000
+            );
+        }
+
+        Notification.notificationRefreshTimeout.start();
+    }
 }

app/models/question.rb

@@ -17,6 +17,9 @@
 #  saved_annotation_id :bigint
 #
 class Question < Annotation
+  belongs_to :user, inverse_of: :questions
+  counter_culture :user, column_name: proc { |question| question.answered? ? nil : 'open_questions_count' }
+
   after_commit :clear_transition
 
   enum question_state: { unanswered: 0, in_progress: 1, answered: 2 }

app/models/user.rb

@@ -2,21 +2,22 @@
 #
 # Table name: users
 #
-#  id             :integer          not null, primary key
-#  username       :string(255)
-#  first_name     :string(255)
-#  last_name      :string(255)
-#  email          :string(255)
-#  permission     :integer          default("student")
-#  created_at     :datetime         not null
-#  updated_at     :datetime         not null
-#  lang           :string(255)      default("nl")
-#  token          :string(255)
-#  time_zone      :string(255)      default("Brussels")
-#  institution_id :bigint
-#  search         :string(4096)
-#  seen_at        :datetime
-#  sign_in_at     :datetime
+#  id                   :integer          not null, primary key
+#  username             :string(255)
+#  first_name           :string(255)
+#  last_name            :string(255)
+#  email                :string(255)
+#  permission           :integer          default("student")
+#  created_at           :datetime         not null
+#  updated_at           :datetime         not null
+#  lang                 :string(255)      default("nl")
+#  token                :string(255)
+#  time_zone            :string(255)      default("Brussels")
+#  institution_id       :bigint
+#  search               :string(4096)
+#  seen_at              :datetime
+#  sign_in_at           :datetime
+#  open_questions_count :integer          default(0), not null
 #
 
 require 'securerandom'
@@ -537,6 +538,10 @@ def jump_back_in
     result
   end
 
+  def open_questions?
+    open_questions_count > 0
+  end
+
   private
 
   def set_token

app/views/layouts/_navbar.html.erb

@@ -34,9 +34,11 @@
               <div class="dropdown-menu dropdown-menu-end notification-dropdown">
                 <%= render partial: 'notifications/small_notifications_table', locals: { notifications: @last_notifications } %>
               </div>
-              <script>
-                dodona.ready.then(function() { setInterval(dodona.Notification.checkNotifications, 60 * 1000); })
-              </script>
+              <% if current_user.open_questions? %>
+                <script>
+                  dodona.ready.then(dodona.Notification.startNotificationRefresh);
+                </script>
+              <% end %>
             </li>
           <% end %>
           <li class="dropdown">

db/migrate/20230130132327_add_open_questions_count_to_users.rb

@@ -0,0 +1,14 @@
+class AddOpenQuestionsCountToUsers < ActiveRecord::Migration[7.0]
+  def self.up
+    add_column :users, :open_questions_count, :integer, null: false, default: 0
+
+    # Should be run manually in the console
+    # User.find_each do |u|
+    #   u.update(open_questions_count: u.questions.where.not(question_state: :answered).count)
+    # end
+  end
+
+  def self.down
+    remove_column :users, :open_questions_count
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_01_27_090704) do
+ActiveRecord::Schema[7.0].define(version: 2023_01_30_132327) do
   create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
     t.string "name", null: false
     t.string "record_type", null: false
@@ -519,6 +519,7 @@
     t.string "search", limit: 4096
     t.datetime "seen_at", precision: nil
     t.datetime "sign_in_at", precision: nil
+    t.integer "open_questions_count", default: 0, null: false
     t.index ["email", "institution_id"], name: "index_users_on_email_and_institution_id", unique: true
     t.index ["institution_id"], name: "index_users_on_institution_id"
     t.index ["token"], name: "index_users_on_token"

test/factories/users.rb

@@ -2,21 +2,22 @@
 #
 # Table name: users
 #
-#  id             :integer          not null, primary key
-#  username       :string(255)
-#  first_name     :string(255)
-#  last_name      :string(255)
-#  email          :string(255)
-#  permission     :integer          default("student")
-#  created_at     :datetime         not null
-#  updated_at     :datetime         not null
-#  lang           :string(255)      default("nl")
-#  token          :string(255)
-#  time_zone      :string(255)      default("Brussels")
-#  institution_id :bigint
-#  search         :string(4096)
-#  seen_at        :datetime
-#  sign_in_at     :datetime
+#  id                   :integer          not null, primary key
+#  username             :string(255)
+#  first_name           :string(255)
+#  last_name            :string(255)
+#  email                :string(255)
+#  permission           :integer          default("student")
+#  created_at           :datetime         not null
+#  updated_at           :datetime         not null
+#  lang                 :string(255)      default("nl")
+#  token                :string(255)
+#  time_zone            :string(255)      default("Brussels")
+#  institution_id       :bigint
+#  search               :string(4096)
+#  seen_at              :datetime
+#  sign_in_at           :datetime
+#  open_questions_count :integer          default(0), not null
 #
 
 FactoryBot.define do

test/fixtures/users.yml

@@ -2,21 +2,22 @@
 #
 # Table name: users
 #
-#  id             :integer          not null, primary key
-#  username       :string(255)
-#  first_name     :string(255)
-#  last_name      :string(255)
-#  email          :string(255)
-#  permission     :integer          default("student")
-#  created_at     :datetime         not null
-#  updated_at     :datetime         not null
-#  lang           :string(255)      default("nl")
-#  token          :string(255)
-#  time_zone      :string(255)      default("Brussels")
-#  institution_id :bigint
-#  search         :string(4096)
-#  seen_at        :datetime
-#  sign_in_at     :datetime
+#  id                   :integer          not null, primary key
+#  username             :string(255)
+#  first_name           :string(255)
+#  last_name            :string(255)
+#  email                :string(255)
+#  permission           :integer          default("student")
+#  created_at           :datetime         not null
+#  updated_at           :datetime         not null
+#  lang                 :string(255)      default("nl")
+#  token                :string(255)
+#  time_zone            :string(255)      default("Brussels")
+#  institution_id       :bigint
+#  search               :string(4096)
+#  seen_at              :datetime
+#  sign_in_at           :datetime
+#  open_questions_count :integer          default(0), not null
 #
 
 zeus:

test/models/user_test.rb

@@ -2,21 +2,22 @@
 #
 # Table name: users
 #
-#  id             :integer          not null, primary key
-#  username       :string(255)
-#  first_name     :string(255)
-#  last_name      :string(255)
-#  email          :string(255)
-#  permission     :integer          default("student")
-#  created_at     :datetime         not null
-#  updated_at     :datetime         not null
-#  lang           :string(255)      default("nl")
-#  token          :string(255)
-#  time_zone      :string(255)      default("Brussels")
-#  institution_id :bigint
-#  search         :string(4096)
-#  seen_at        :datetime
-#  sign_in_at     :datetime
+#  id                   :integer          not null, primary key
+#  username             :string(255)
+#  first_name           :string(255)
+#  last_name            :string(255)
+#  email                :string(255)
+#  permission           :integer          default("student")
+#  created_at           :datetime         not null
+#  updated_at           :datetime         not null
+#  lang                 :string(255)      default("nl")
+#  token                :string(255)
+#  time_zone            :string(255)      default("Brussels")
+#  institution_id       :bigint
+#  search               :string(4096)
+#  seen_at              :datetime
+#  sign_in_at           :datetime
+#  open_questions_count :integer          default(0), not null
 #
 
 require 'test_helper'
@@ -1038,4 +1039,35 @@ def setup
     assert_equal 3, user.jump_back_in.count
     assert_equal a1, user.jump_back_in.first[:activity]
   end
+
+  test 'Open questions count gets updated when a new question is created' do
+    user = create :user
+    assert_equal 0, user.open_questions_count
+    create :question, submission: create(:submission, user: user, course: create(:course))
+    assert_equal 1, user.reload.open_questions_count
+    s = create :submission, user: user, course: create(:course)
+    create :question, submission: s
+    assert_equal 2, user.reload.open_questions_count
+    create :question, submission: s
+    assert_equal 3, user.reload.open_questions_count
+    create :question, submission: create(:submission, user: create(:user), course: create(:course))
+    assert_equal 3, user.reload.open_questions_count
+  end
+
+  test 'Open questions count gets updated when a question is answered' do
+    user = create :user
+    question = create :question, submission: create(:submission, user: user, course: create(:course))
+    assert_equal 1, user.reload.open_questions_count
+    question.update(question_state: :answered)
+    assert_equal 0, user.reload.open_questions_count
+  end
+
+  test 'has open questions is always correct' do
+    user = create :user
+    assert_equal false, user.open_questions?
+    question = create :question, submission: create(:submission, user: user, course: create(:course))
+    assert_equal true, user.reload.open_questions?
+    question.update(question_state: :answered)
+    assert_equal false, user.reload.open_questions?
+  end
 end