From f636d0fe322b28f02e5d427608cc05f7593cb0a6 Mon Sep 17 00:00:00 2001 From: Splines <37160523+Splines@users.noreply.github.com> Date: Thu, 11 Jan 2024 19:05:36 +0100 Subject: [PATCH 1/7] Warn about too long GitHub commit messages (#586) --- .vscode/settings.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.vscode/settings.json b/.vscode/settings.json index 21983ff1e..24b00c4b5 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -79,6 +79,12 @@ } ], ////////////////////////////////////// + // Git + ////////////////////////////////////// + "git.inputValidation": "warn", + "git.inputValidationSubjectLength": 50, + "git.inputValidationLength": 72, + ////////////////////////////////////// // Spell Checker ////////////////////////////////////// "cSpell.words": [ From eb9484a8106d15683c5028b55acb0d6798329c29 Mon Sep 17 00:00:00 2001 From: Splines <37160523+Splines@users.noreply.github.com> Date: Tue, 16 Jan 2024 19:15:51 +0100 Subject: [PATCH 2/7] Fix comment status (#585) * Reapply first fix for Reader/Media See discussion on #574 for further details. Previous PR for this was #576, closed in favor of this one as this directly branches off the new "dev" branch. * Correctly show latest post (might be current_user's comment) * Fix update of unread comments logic in comments controller * Fix update icon logic and latest post comment * Simplify latest comment logic * Improve code comments * Further improve comments * Fix wording in comment * Fix construction of media array & use `.blank?` instead of `.empty?` --- .../commontator/comments_controller.rb | 51 +++++++++++++------ app/controllers/main_controller.rb | 2 +- app/controllers/readers_controller.rb | 2 +- app/models/user.rb | 38 +++++++++++--- app/views/main/comments.html.erb | 4 +- 5 files changed, 70 insertions(+), 27 deletions(-) diff --git a/app/controllers/commontator/comments_controller.rb b/app/controllers/commontator/comments_controller.rb index 31058b3da..331919345 100644 --- a/app/controllers/commontator/comments_controller.rb +++ b/app/controllers/commontator/comments_controller.rb @@ -71,6 +71,7 @@ def create Commontator::Subscription.comment_created(@comment) # The next line constitutes a customization of the original controller update_unread_status + activate_unread_comments_icon_if_necessary @commontator_page = @commontator_thread.new_comment_page( @comment.parent_id, @commontator_show_all @@ -194,9 +195,14 @@ def subscribe_mentioned end end - # This method ensures that the unread_comments flag is updated - # for users affected by the creation of a newly created comment - # It constitues a customization + # Updates the unread_comments flag for users subscribed to the current thread. + # This method should only be called when a new comment was created. + # + # The originator of the comment does not get the flag set since that user + # already knows about the comment; that user has just created it after all. + # + # (This is a customization of the original controller provided + # by the commontator gem.) def update_unread_status medium = @commontator_thread.commontable return unless medium.released.in?(["all", "users", "subscribers"]) @@ -205,22 +211,35 @@ def update_unread_status relevant_users.where.not(id: current_user.id) .where(unread_comments: false) .update(unread_comments: true) - - # make sure that the thread associated to this comment is marked as read - # by the comment creator (unless some other user posted a comment in it - # that has not yet been read) - @reader = Reader.find_or_create_by(user: current_user, - thread: @commontator_thread) - if unseen_comments? - @update_icon = true - return - end - @reader.touch end - def unseen_comments? + # Might activate the flag used in the view to indicate unread comments. + # This method should only be called when a new comment was created. + # The flag is activated if the current user has not seen all comments + # in the thread in which the new comment was created. + # + # The flag might only be activated, not deactivated since the checks + # performed here are not sufficient to determine whether a user has + # any unread comments (including those in possibly other threads). + # + # This method was introduced for one specific edge case: + # When the current user A has just created a new comment in a thread, + # but in the meantime, another user B has created a comment in the same + # thread. User A will not be informed immediately about the new comment + # by B since we don't have websockets implemented. Instead, A will be + # informed by a visual indicator as soon as A has posted their own comment. + # + # (This is a customization of the original controller provided + # by the commontator gem.) + def activate_unread_comments_icon_if_necessary + reader = Reader.find_by(user: current_user, thread: @commontator_thread) + @update_icon = true if unseen_comments_in_current_thread?(reader) + end + + def unseen_comments_in_current_thread?(reader = nil) @commontator_thread.comments.any? do |c| - c.creator != current_user && c.created_at > @reader.updated_at + not_marked_as_read_in_reader = reader.nil? || c.created_at > reader.updated_at + c.creator != current_user && not_marked_as_read_in_reader end end end diff --git a/app/controllers/main_controller.rb b/app/controllers/main_controller.rb index 8fa45237c..c7e8d469b 100644 --- a/app/controllers/main_controller.rb +++ b/app/controllers/main_controller.rb @@ -26,7 +26,7 @@ def sponsors end def comments - @media_comments = current_user.media_latest_comments + @media_comments = current_user.subscribed_media_with_latest_comments_not_by_creator @media_comments.select! do |m| (Reader.find_by(user: current_user, thread: m[:thread]) &.updated_at || 1000.years.ago) < m[:latest_comment].created_at && diff --git a/app/controllers/readers_controller.rb b/app/controllers/readers_controller.rb index f276f2989..931b93a45 100644 --- a/app/controllers/readers_controller.rb +++ b/app/controllers/readers_controller.rb @@ -9,7 +9,7 @@ def update @reader = Reader.find_or_create_by(user: current_user, thread: @thread) @reader.touch - @anything_left = current_user.media_latest_comments.any? do |m| + @anything_left = current_user.subscribed_media_with_latest_comments_not_by_creator.any? do |m| (Reader.find_by(user: current_user, thread: m[:thread]) &.updated_at || 1000.years.ago) < m[:latest_comment].created_at end diff --git a/app/models/user.rb b/app/models/user.rb index 7745f2260..40feb6f60 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -549,14 +549,38 @@ def subscribed_commentable_media_with_comments .select { |m| m.commontator_thread.comments.any? } end - def media_latest_comments - media = subscribed_commentable_media_with_comments - .map do |m| - { medium: m, - thread: m.commontator_thread, - latest_comment: m.commontator_thread - .comments.max_by(&:created_at) } + # Returns the media that the user has subscribed to and that have been + # commented on by somebody else (not by the current user). The order is + # given by the time of the latest comment by somebody else. + # + # Media that have not been commented on by somebody else than the current user, + # are not returned (!). + # + # For each medium, the following information is stored: + # - the medium itself + # - the thread of the medium + # - the latest comment by somebody else than the current user + # - the latest comment by any user (which might include the current user) + def subscribed_media_with_latest_comments_not_by_creator + media = [] + + subscribed_commentable_media_with_comments.each do |m| + thread = m.commontator_thread + comments = thread.comments + next if comments.blank? + + comments_not_by_creator = comments.reject { |c| c.creator == self } + next if comments_not_by_creator.blank? + + latest_comment = comments_not_by_creator.max_by(&:created_at) + latest_comment_by_any_user = comments.max_by(&:created_at) + + media << { medium: m, + thread: thread, + latest_comment: latest_comment, + latest_comment_by_any_user: latest_comment_by_any_user } end + media.sort_by { |x| x[:latest_comment].created_at }.reverse end diff --git a/app/views/main/comments.html.erb b/app/views/main/comments.html.erb index 13e122bad..22f6e5ad4 100644 --- a/app/views/main/comments.html.erb +++ b/app/views/main/comments.html.erb @@ -68,8 +68,8 @@
<%= t('comments.who_and_when', - when: time_ago_in_words(m[:latest_comment].created_at), - who: m[:latest_comment].creator.name) %> + when: time_ago_in_words(m[:latest_comment_by_any_user].created_at), + who: m[:latest_comment_by_any_user].creator.name) %>
From 475fc0483821e89df8f2c1f73772c11b71920893 Mon Sep 17 00:00:00 2001 From: Splines <37160523+Splines@users.noreply.github.com> Date: Wed, 17 Jan 2024 23:21:59 +0100 Subject: [PATCH 3/7] Migrate `unread_comments` flag (fix inconsistencies) (#587) * Add dummy migration * Implement migration for unread comment flag * Remove unnecessary comment * Declare migration as not idempotent * Use array.length instead of counting * Throw error to prevent revert of migration * Fix severe flaws in unread comments migration * Simplify Reader retrieval * Use the more explicit `.nil?` method * Update migration date * Fix annoying bug: don't use `.select!` but `.select` * Polish migration e.g. update comment, more suitable name for the method etc. * Rename method according to #585 --- ...000_fix_unread_comments_inconsistencies.rb | 56 +++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb diff --git a/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb b/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb new file mode 100644 index 000000000..3392da3dc --- /dev/null +++ b/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb @@ -0,0 +1,56 @@ +# Fixes the unread_comments flag for all users. Unintended behavior was +# introduced in pull request #515. Migration introduced in #587. +# Behavior fixed in #585. +# +# This migration is generally *not* idempotent since users might have interacted +# with the website since the migration was run and thus they will probably have +# different unread comments flags as the ones at the time of the migration. +# +# This migration is not reversible as we don't store the previous state of +# the unread_comments flag. +class FixUnreadCommentsInconsistencies < ActiveRecord::Migration[7.0] + def up + num_fixed_users = 0 + + User.find_each do |user| + had_user_unread_comments = user.unread_comments # boolean + has_user_unread_comments = user_unread_comments?(user) + + has_flag_changed = (had_user_unread_comments != has_user_unread_comments) + user.update(unread_comments: has_user_unread_comments) if has_flag_changed + num_fixed_users += 1 if has_flag_changed + end + + Rails.logger.debug { "Ran through #{User.count} users (unread comments flag)" } + Rails.logger.debug { "Fixed #{num_fixed_users} users (unread comments flag)" } + end + + # Checks and returns whether the user has unread comments. + def user_unread_comments?(user) + # Check for unread comments -- directly via Reader + readers = Reader.where(user: user) + readers.each do |reader| + thread = Commontator::Thread.find_by(id: reader.thread_id) + next if thread.nil? + + latest_thread_comment_by_any_user = thread.comments.max_by(&:created_at) + next if latest_thread_comment_by_any_user.blank? + + latest_thread_comment_time = latest_thread_comment_by_any_user.created_at + has_user_unread_comments = reader.updated_at < latest_thread_comment_time + + return true if has_user_unread_comments + end + + # User might still have unread comments but no related Reader objects + # -> Check for unread comments -- via Media + unseen_media = user.subscribed_media_with_latest_comments_not_by_creator.select do |m| + m[:medium].visible_for_user?(user) + end + unseen_media.present? + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index 923aed6fb..0eb8c6842 100644 --- a/db/schema.rb +++ b/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_11_01_100015) do +ActiveRecord::Schema[7.0].define(version: 2024_01_16_180000) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" From 92d48d4d03f1393fb017356384afee0b8cdb6ace Mon Sep 17 00:00:00 2001 From: Splines <37160523+Splines@users.noreply.github.com> Date: Fri, 19 Jan 2024 16:06:36 +0100 Subject: [PATCH 4/7] Use `warn` log level for migration (#588) --- .../20240116180000_fix_unread_comments_inconsistencies.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb b/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb index 3392da3dc..74ef88755 100644 --- a/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb +++ b/db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb @@ -21,8 +21,8 @@ def up num_fixed_users += 1 if has_flag_changed end - Rails.logger.debug { "Ran through #{User.count} users (unread comments flag)" } - Rails.logger.debug { "Fixed #{num_fixed_users} users (unread comments flag)" } + Rails.logger.warn { "Ran through #{User.count} users (unread comments flag)" } + Rails.logger.warn { "Fixed #{num_fixed_users} users (unread comments flag)" } end # Checks and returns whether the user has unread comments. From aeeb53cfee43f60867bb86e7c63258f28828aea8 Mon Sep 17 00:00:00 2001 From: Splines Date: Mon, 12 Feb 2024 16:02:51 +0100 Subject: [PATCH 5/7] Authenticate mails sent by MaMpf --- config/environments/production.rb | 6 ++++-- docker/production/docker.env | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/config/environments/production.rb b/config/environments/production.rb index 805b5c66e..da185fce5 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -69,9 +69,11 @@ config.action_mailer.default(charset: "utf-8") config.action_mailer.smtp_settings = { - address: ENV.fetch("MAILSERVER", nil), + address: ENV.fetch("MAILSERVER"), + domain: ENV.fetch("MAILSERVER"), port: 25, - domain: ENV.fetch("MAILSERVER", nil) + user_name: ENV.fetch("MAMPF_EMAIL_USERNAME"), + password: ENV.fetch("MAMPF_EMAIL_PASSWORD") } # Ignore bad email addresses and do not raise email delivery errors. diff --git a/docker/production/docker.env b/docker/production/docker.env index 8070eb0d9..58920ef10 100644 --- a/docker/production/docker.env +++ b/docker/production/docker.env @@ -21,6 +21,9 @@ PROJECT_EMAIL_USERNAME=creativeusername PROJECT_EMAIL_PASSWORD=secretsecret PROJECT_EMAIL_MAILBOX=Other Users/mampf +MAMPF_EMAIL_USERNAME=creativeusername +MAMPF_EMAIL_PASSWORD=creativepassword + # Due to CORS constraints, some urls are proxied to the media server DOWNLOAD_LOCATION=https://mampf.mathi.uni-heidelberg.de/mediaforward From 2bb15cd1770791e0a8e586e48098fd7366b1d150 Mon Sep 17 00:00:00 2001 From: Splines Date: Mon, 12 Feb 2024 16:09:09 +0100 Subject: [PATCH 6/7] Fix mails comment --- docker/production/docker.env | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docker/production/docker.env b/docker/production/docker.env index 58920ef10..373249f82 100644 --- a/docker/production/docker.env +++ b/docker/production/docker.env @@ -10,7 +10,7 @@ MUESLI_SERVER=https://muesli.mathi.uni-heidelberg.de ERDBEERE_API=https://erdbeere.mathi.uni-heidelberg.de/api/v1 MEMCACHED_SERVER=cache -# Email is send using a mailserver without authentication. Specify how to connect here +# Email FROM_ADDRESS=mampf@mathi.uni-heidelberg.de MAILSERVER=mail.mathi.uni-heidelberg.de PROJECT_EMAIL=mampf@mathi.uni-heidelberg.de @@ -20,7 +20,6 @@ IMAPSERVER=mail.mathi.uni-heidelberg.de PROJECT_EMAIL_USERNAME=creativeusername PROJECT_EMAIL_PASSWORD=secretsecret PROJECT_EMAIL_MAILBOX=Other Users/mampf - MAMPF_EMAIL_USERNAME=creativeusername MAMPF_EMAIL_PASSWORD=creativepassword From f40080ed1311ae0bc01088899cd68677ced435c1 Mon Sep 17 00:00:00 2001 From: Splines Date: Tue, 13 Feb 2024 12:17:08 +0100 Subject: [PATCH 7/7] Add `nil` env fallback due to #542 --- config/environments/production.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config/environments/production.rb b/config/environments/production.rb index da185fce5..3e423268f 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -69,11 +69,11 @@ config.action_mailer.default(charset: "utf-8") config.action_mailer.smtp_settings = { - address: ENV.fetch("MAILSERVER"), - domain: ENV.fetch("MAILSERVER"), + address: ENV.fetch("MAILSERVER", nil), + domain: ENV.fetch("MAILSERVER", nil), port: 25, - user_name: ENV.fetch("MAMPF_EMAIL_USERNAME"), - password: ENV.fetch("MAMPF_EMAIL_PASSWORD") + user_name: ENV.fetch("MAMPF_EMAIL_USERNAME", nil), + password: ENV.fetch("MAMPF_EMAIL_PASSWORD", nil) } # Ignore bad email addresses and do not raise email delivery errors.