Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the number of notifications requests #4373

Merged
merged 13 commits into from
Jan 31, 2023
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
4 changes: 4 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion app/assets/javascripts/auto_reload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down
4 changes: 4 additions & 0 deletions app/assets/javascripts/code_listing/question_annotation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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();
Expand Down
15 changes: 15 additions & 0 deletions app/assets/javascripts/notification.ts
Original file line number Diff line number Diff line change
@@ -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:
Expand Down Expand Up @@ -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();
}
}
3 changes: 3 additions & 0 deletions app/models/question.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
35 changes: 20 additions & 15 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -537,6 +538,10 @@ def jump_back_in
result
end

def open_questions?
open_questions_count > 0
end

private

def set_token
Expand Down
8 changes: 5 additions & 3 deletions app/views/layouts/_navbar.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
14 changes: 14 additions & 0 deletions db/migrate/20230130132327_add_open_questions_count_to_users.rb
Original file line number Diff line number Diff line change
@@ -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
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
31 changes: 16 additions & 15 deletions test/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 16 additions & 15 deletions test/fixtures/users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
62 changes: 47 additions & 15 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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