From 09d110eb581d9c3ae8a68249e4075bf0ca418087 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 13:03:06 +0100 Subject: [PATCH 01/10] Add option for allowing long questions --- .../20230108114333_add_allow_long_questions_to_profiles.rb | 7 +++++++ db/schema.rb | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20230108114333_add_allow_long_questions_to_profiles.rb diff --git a/db/migrate/20230108114333_add_allow_long_questions_to_profiles.rb b/db/migrate/20230108114333_add_allow_long_questions_to_profiles.rb new file mode 100644 index 00000000..5f67a004 --- /dev/null +++ b/db/migrate/20230108114333_add_allow_long_questions_to_profiles.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddAllowLongQuestionsToProfiles < ActiveRecord::Migration[6.1] + def change + add_column :profiles, :allow_long_questions, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 05514255..45b1005d 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.define(version: 2022_12_27_065923) do +ActiveRecord::Schema.define(version: 2023_01_08_114333) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -134,6 +134,7 @@ ActiveRecord::Schema.define(version: 2022_12_27_065923) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "anon_display_name" + t.boolean "allow_long_questions", default: false t.index ["user_id"], name: "index_profiles_on_user_id" end From ba7b19faeeba23138501aff4f1ac0d15e1acb552 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 13:13:03 +0100 Subject: [PATCH 02/10] Add allow long questions option to profile settings --- app/controllers/settings/profile_controller.rb | 2 +- app/views/settings/profile/edit.html.haml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/settings/profile_controller.rb b/app/controllers/settings/profile_controller.rb index 2d1576dd..75091ed0 100644 --- a/app/controllers/settings/profile_controller.rb +++ b/app/controllers/settings/profile_controller.rb @@ -6,7 +6,7 @@ class Settings::ProfileController < ApplicationController def edit; end def update - profile_attributes = params.require(:profile).permit(:display_name, :motivation_header, :website, :location, :description, :anon_display_name) + profile_attributes = params.require(:profile).permit(:display_name, :motivation_header, :website, :location, :description, :anon_display_name, :allow_long_questions) if current_user.profile.update(profile_attributes) flash[:success] = t(".success") diff --git a/app/views/settings/profile/edit.html.haml b/app/views/settings/profile/edit.html.haml index 1f8240d4..7cd29926 100644 --- a/app/views/settings/profile/edit.html.haml +++ b/app/views/settings/profile/edit.html.haml @@ -49,6 +49,8 @@ = f.text_area :description + = f.check_box :allow_long_questions + = f.primary - provide(:title, generate_title(t(".title"))) From e2f6284982fcddd6ddedfea03005c9b488cd1e23 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 14:00:16 +0100 Subject: [PATCH 03/10] Update user exporter spec to include `allow_long_questions` field --- spec/lib/use_case/data_export/user_spec.rb | 28 ++++++++++++---------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/spec/lib/use_case/data_export/user_spec.rb b/spec/lib/use_case/data_export/user_spec.rb index a8c692ea..d04621c5 100644 --- a/spec/lib/use_case/data_export/user_spec.rb +++ b/spec/lib/use_case/data_export/user_spec.rb @@ -27,11 +27,12 @@ describe UseCase::DataExport::User, :data_export do sign_in_count: 10, smiled_count: 28, profile: { - display_name: "Fizzy Raccoon", - description: "A small raccoon", - location: "Binland", - motivation_header: "", - website: "https://retrospring.net" + display_name: "Fizzy Raccoon", + description: "A small raccoon", + location: "Binland", + motivation_header: "", + website: "https://retrospring.net", + allow_long_questions: true } } end @@ -87,14 +88,15 @@ describe UseCase::DataExport::User, :data_export do privacy_noindex: false }, profile: { - display_name: "Fizzy Raccoon", - description: "A small raccoon", - location: "Binland", - website: "https://retrospring.net", - motivation_header: "", - created_at: user.profile.created_at.as_json, - updated_at: user.profile.updated_at.as_json, - anon_display_name: nil + display_name: "Fizzy Raccoon", + description: "A small raccoon", + location: "Binland", + website: "https://retrospring.net", + motivation_header: "", + created_at: user.profile.created_at.as_json, + updated_at: user.profile.updated_at.as_json, + anon_display_name: nil, + allow_long_questions: true, }, roles: { administrator: false, From 3a6814b9086956088c774274ef82b90b7bca8fb1 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 14:03:54 +0100 Subject: [PATCH 04/10] Respect allow long questions setting --- app/models/question.rb | 6 +++++- app/workers/question_worker.rb | 1 + lib/errors.rb | 3 +++ lib/use_case/question/create.rb | 1 + spec/lib/use_case/question/create_spec.rb | 2 +- spec/models/question_spec.rb | 5 ----- 6 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index 88b5565c..0a6adec0 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -5,7 +5,9 @@ class Question < ApplicationRecord has_many :answers, dependent: :destroy has_many :inboxes, dependent: :destroy - validates :content, length: { minimum: 1, maximum: 512 } + validates :content, length: { minimum: 1 } + + SHORT_QUESTION_MAX_LENGTH = 512 before_destroy do rep = Report.where(target_id: self.id, type: 'Reports::Question') @@ -30,4 +32,6 @@ class Question < ApplicationRecord def generated? = %w[justask retrospring_exporter].include?(author_identifier) def anonymous? = author_is_anonymous && author_identifier.present? + + def long? = content.length > SHORT_QUESTION_MAX_LENGTH end diff --git a/app/workers/question_worker.rb b/app/workers/question_worker.rb index c30c98d4..5d10904f 100644 --- a/app/workers/question_worker.rb +++ b/app/workers/question_worker.rb @@ -30,6 +30,7 @@ class QuestionWorker return true if follower.banned? return true if muted?(follower, question) return true if user.muting?(question.user) + return true if question.long? && !user.profile.allow_long_questions false end diff --git a/lib/errors.rb b/lib/errors.rb index 314ed4bc..5e2586c8 100644 --- a/lib/errors.rb +++ b/lib/errors.rb @@ -28,6 +28,9 @@ module Errors class InvalidBanDuration < BadRequest end + class QuestionTooLong < BadRequest + end + class Forbidden < Base def status 403 diff --git a/lib/use_case/question/create.rb b/lib/use_case/question/create.rb index a09a1ff7..51a4f3de 100644 --- a/lib/use_case/question/create.rb +++ b/lib/use_case/question/create.rb @@ -63,6 +63,7 @@ module UseCase def check_user raise Errors::NotAuthorized if target_user.privacy_require_user && !source_user_id + raise Errors::QuestionTooLong if content.length > ::Question::SHORT_QUESTION_MAX_LENGTH && !target_user.profile.allow_long_questions end def create_question diff --git a/spec/lib/use_case/question/create_spec.rb b/spec/lib/use_case/question/create_spec.rb index 607123b3..47b80ea4 100644 --- a/spec/lib/use_case/question/create_spec.rb +++ b/spec/lib/use_case/question/create_spec.rb @@ -61,7 +61,7 @@ describe UseCase::Question::Create do let(:content) { "a" * 513 } it "raises an error" do - expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + expect { subject }.to raise_error(Errors::QuestionTooLong) end end end diff --git a/spec/models/question_spec.rb b/spec/models/question_spec.rb index 2e88fc42..7da9eab0 100644 --- a/spec/models/question_spec.rb +++ b/spec/models/question_spec.rb @@ -16,11 +16,6 @@ RSpec.describe Question, :type => :model do expect(@question.content).to match 'Is this a question?' end - it 'does not save questions longer than 512 characters' do - @question.content = 'X' * 513 - expect{@question.save!}.to raise_error(ActiveRecord::RecordInvalid) - end - it 'has many answers' do 5.times { |i| Answer.create(content: "This is an answer. #{i}", user: FactoryBot.create(:user), question: @question) } expect(@question.answer_count).to match 5 From 7aacb1a36429fa95ee1574b45947731d2378e9f8 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 14:51:37 +0100 Subject: [PATCH 05/10] Test creating question when recipient allows long questions --- spec/lib/use_case/question/create_spec.rb | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/spec/lib/use_case/question/create_spec.rb b/spec/lib/use_case/question/create_spec.rb index 47b80ea4..a1831771 100644 --- a/spec/lib/use_case/question/create_spec.rb +++ b/spec/lib/use_case/question/create_spec.rb @@ -57,11 +57,22 @@ describe UseCase::Question::Create do end end - context "content is too long" do + context "content is over 512 characters long" do let(:content) { "a" * 513 } - it "raises an error" do - expect { subject }.to raise_error(Errors::QuestionTooLong) + context "recipient does not allow long questions" do + it "raises an error" do + expect { subject }.to raise_error(Errors::QuestionTooLong) + end + end + + context "recipient allows long questions" do + before do + target_user.profile.allow_long_questions = true + target_user.profile.save + end + + it_behaves_like "creates the question" end end end From 7cdb0e4976c3d84b4edc18a9b57a49021b3571bf Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 17:12:46 +0100 Subject: [PATCH 06/10] Respect long question setting in frontend --- .../character_count_warning_controller.ts | 30 +++++++++++++++++++ .../retrospring/initializers/stimulus.ts | 2 ++ app/models/profile.rb | 2 ++ app/views/application/_questionbox.html.haml | 4 +-- app/views/modal/_ask.html.haml | 8 ++--- config/locales/views.en.yml | 1 + 6 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 app/javascript/retrospring/controllers/character_count_warning_controller.ts diff --git a/app/javascript/retrospring/controllers/character_count_warning_controller.ts b/app/javascript/retrospring/controllers/character_count_warning_controller.ts new file mode 100644 index 00000000..8c1bfe07 --- /dev/null +++ b/app/javascript/retrospring/controllers/character_count_warning_controller.ts @@ -0,0 +1,30 @@ +import { Controller } from '@hotwired/stimulus'; + +export default class extends Controller { + static targets = ['input', 'warning']; + + declare readonly inputTarget: HTMLInputElement; + declare readonly warningTarget: HTMLElement; + + static values = { + warn: Number + }; + + declare readonly warnValue: number; + + connect(): void { + this.inputTarget.addEventListener('input', this.update.bind(this)); + } + + update(): void { + if (this.inputTarget.value.length > this.warnValue) { + if (this.warningTarget.classList.contains('d-none')) { + this.warningTarget.classList.remove('d-none'); + } + } else { + if (!this.warningTarget.classList.contains('d-none')) { + this.warningTarget.classList.add('d-none'); + } + } + } +} diff --git a/app/javascript/retrospring/initializers/stimulus.ts b/app/javascript/retrospring/initializers/stimulus.ts index fc65f20e..09ead0bf 100644 --- a/app/javascript/retrospring/initializers/stimulus.ts +++ b/app/javascript/retrospring/initializers/stimulus.ts @@ -2,6 +2,7 @@ import { Application } from "@hotwired/stimulus"; import AnnouncementController from "retrospring/controllers/announcement_controller"; import AutofocusController from "retrospring/controllers/autofocus_controller"; import CharacterCountController from "retrospring/controllers/character_count_controller"; +import CharacterCountWarningController from "retrospring/controllers/character_count_warning_controller"; /** * This module sets up Stimulus and our controllers @@ -15,4 +16,5 @@ export default function (): void { window['Stimulus'].register('announcement', AnnouncementController); window['Stimulus'].register('autofocus', AutofocusController); window['Stimulus'].register('character-count', CharacterCountController); + window['Stimulus'].register('character-count-warning', CharacterCountWarningController); } diff --git a/app/models/profile.rb b/app/models/profile.rb index df75ce61..b2927860 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -26,4 +26,6 @@ class Profile < ApplicationRecord def safe_name display_name.presence || user.screen_name end + + def question_length_limit = allow_long_questions ? nil : Question::SHORT_QUESTION_MAX_LENGTH end diff --git a/app/views/application/_questionbox.html.haml b/app/views/application/_questionbox.html.haml index 42c0731e..d56fefda 100644 --- a/app/views/application/_questionbox.html.haml +++ b/app/views/application/_questionbox.html.haml @@ -22,7 +22,7 @@ %strong= t(".status.require_user_html", sign_in: link_to(t("voc.login"), new_user_session_path), sign_up: link_to(t("voc.register"), new_user_registration_path)) - else - if user_signed_in? || user.privacy_allow_anonymous_questions? - #question-box{ data: { controller: "character-count", "character-count-max-value": 512 }} + #question-box{ data: user.profile.allow_long_questions ? {} : { controller: "character-count", "character-count-max-value": user.profile.question_length_limit }} %textarea.form-control{ name: "qb-question", placeholder: t(".placeholder"), data: { "character-count-target": "input" } } .row{ style: "padding-top: 5px;" } .col-6 @@ -36,7 +36,7 @@ %input{ name: "qb-anonymous", type: :hidden, value: false }/ .col-6 %p.pull-right - %span.text-muted.me-1{ data: { "character-count-target": "counter" } } 512 + %span.text-muted.me-1{ class: user.profile.allow_long_questions ? "d-none" : "", data: { "character-count-target": "counter" } }= Question::SHORT_QUESTION_MAX_LENGTH %input{ name: "qb-to", type: "hidden", value: user.id }/ %button.btn.btn-primary{ name: "qb-ask", type: :button, diff --git a/app/views/modal/_ask.html.haml b/app/views/modal/_ask.html.haml index 740a684a..00192b61 100644 --- a/app/views/modal/_ask.html.haml +++ b/app/views/modal/_ask.html.haml @@ -1,14 +1,14 @@ .modal.fade#modal-ask-followers{ aria: { hidden: true, labelledby: "modal-ask-followers-label" }, role: :dialog, tabindex: -1 } .modal-dialog - .modal-content{ data: { controller: "character-count", "character-count-max-value": 512 }} + .modal-content{ data: { controller: "character-count-warning", "character-count-warning-warn-value": Question::SHORT_QUESTION_MAX_LENGTH }} .modal-header %h5.modal-title#modal-ask-followers-label= t(".title") %button.btn-close{ data: { bs_dismiss: :modal }, type: :button } %span.visually-hidden= t("voc.close") .modal-body .form-group.has-feedback - %textarea.form-control{ name: "qb-all-question", placeholder: t(".placeholder"), data: { "character-count-target": "input" } } - %p.text-end.text-muted.form-control-feedback{ data: { "character-count-target": "counter" } } 512 + %textarea.form-control{ name: "qb-all-question", placeholder: t(".placeholder"), data: { "character-count-warning-target": "input" } } + .alert.alert-warning.mt-3.d-none{ data: { "character-count-warning-target": "warning" } }= t('.long_question_warning') .modal-footer %button.btn.btn-default{ type: :button, data: { bs_dismiss: :modal } }= t("voc.cancel") - %button.btn.btn-primary{ name: "qb-all-ask", type: :button, data: { "character-count-target": "action", loading_text: t(".loading") } }= t(".action") + %button.btn.btn-primary{ name: "qb-all-ask", type: :button, data: { loading_text: t(".loading") } }= t(".action") diff --git a/config/locales/views.en.yml b/config/locales/views.en.yml index efe4373a..c0589eea 100644 --- a/config/locales/views.en.yml +++ b/config/locales/views.en.yml @@ -254,6 +254,7 @@ en: placeholder: "Type your question here…" action: "Ask" loading: "Asking…" + long_question_warning: "This question will only be sent to those who allow long questions." comment_smiles: title: "People who smiled this comment" none: "No one has smiled this comment yet." From e3254cba42e746458af264dd56b24f19ad345e9e Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 17:14:47 +0100 Subject: [PATCH 07/10] Remove trailing comma --- spec/lib/use_case/data_export/user_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/use_case/data_export/user_spec.rb b/spec/lib/use_case/data_export/user_spec.rb index d04621c5..f9c70ff9 100644 --- a/spec/lib/use_case/data_export/user_spec.rb +++ b/spec/lib/use_case/data_export/user_spec.rb @@ -96,7 +96,7 @@ describe UseCase::DataExport::User, :data_export do created_at: user.profile.created_at.as_json, updated_at: user.profile.updated_at.as_json, anon_display_name: nil, - allow_long_questions: true, + allow_long_questions: true }, roles: { administrator: false, From 2e7fca67fad8188fe114378d74124f0ad64ec2b2 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 17:20:12 +0100 Subject: [PATCH 08/10] Update long question warning to be more descriptive --- config/locales/views.en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/views.en.yml b/config/locales/views.en.yml index c0589eea..06ebaca6 100644 --- a/config/locales/views.en.yml +++ b/config/locales/views.en.yml @@ -254,7 +254,7 @@ en: placeholder: "Type your question here…" action: "Ask" loading: "Asking…" - long_question_warning: "This question will only be sent to those who allow long questions." + long_question_warning: "This question will only be sent to those who allow long questions in their profile settings." comment_smiles: title: "People who smiled this comment" none: "No one has smiled this comment yet." From cfba963b55c7eb4aef46236caf97cccb2ddc7fcf Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 18:51:43 +0100 Subject: [PATCH 09/10] Only reset character counters when they are present Co-authored-by: Andreas Nedbal --- app/javascript/retrospring/features/questionbox/all.ts | 4 ---- app/javascript/retrospring/features/questionbox/user.ts | 6 ++++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/javascript/retrospring/features/questionbox/all.ts b/app/javascript/retrospring/features/questionbox/all.ts index a7d6a3b8..4a1373f4 100644 --- a/app/javascript/retrospring/features/questionbox/all.ts +++ b/app/javascript/retrospring/features/questionbox/all.ts @@ -24,10 +24,6 @@ export function questionboxAllHandler(event: Event): void { document.querySelector('textarea[name=qb-all-question]').value = ''; const modal = Modal.getInstance(document.querySelector('#modal-ask-followers')); modal.hide(); - - // FIXME: also solve this using a Stimulus controller - const characterCount = document.querySelector('#modal-ask-followers [data-character-count-max-value]').dataset.characterCountMaxValue; - document.querySelector('#modal-ask-followers [data-character-count-target="counter"]').innerHTML = characterCount; } showNotification(data.message, data.success); diff --git a/app/javascript/retrospring/features/questionbox/user.ts b/app/javascript/retrospring/features/questionbox/user.ts index d398c70f..5fa3cbf9 100644 --- a/app/javascript/retrospring/features/questionbox/user.ts +++ b/app/javascript/retrospring/features/questionbox/user.ts @@ -30,8 +30,10 @@ export function questionboxUserHandler(event: Event): void { document.querySelector('textarea[name=qb-question]').value = ''; // FIXME: also solve this using a Stimulus controller - const characterCount = document.querySelector('#question-box[data-character-count-max-value]').dataset.characterCountMaxValue; - document.querySelector('#question-box [data-character-count-target="counter"]').innerHTML = characterCount; + const questionBox = document.getElementById('question-box'); + if ('characterCountMaxValue' in questionBox.dataset) { + questionBox.querySelector('[data-character-count-target="counter"]').innerHTML = questionBox.dataset.characterCountMaxValue; + } if (promote) { const questionbox = document.querySelector('#question-box'); From 4e78efcae73819549feaf7c45bf45f9213f72629 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 8 Jan 2023 19:22:00 +0100 Subject: [PATCH 10/10] Add tests for sending long questions with question worker --- app/workers/question_worker.rb | 2 +- spec/workers/question_worker_spec.rb | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/workers/question_worker.rb b/app/workers/question_worker.rb index 5d10904f..969b725f 100644 --- a/app/workers/question_worker.rb +++ b/app/workers/question_worker.rb @@ -30,7 +30,7 @@ class QuestionWorker return true if follower.banned? return true if muted?(follower, question) return true if user.muting?(question.user) - return true if question.long? && !user.profile.allow_long_questions + return true if question.long? && !follower.profile.allow_long_questions false end diff --git a/spec/workers/question_worker_spec.rb b/spec/workers/question_worker_spec.rb index cf022124..8070392b 100644 --- a/spec/workers/question_worker_spec.rb +++ b/spec/workers/question_worker_spec.rb @@ -6,7 +6,8 @@ describe QuestionWorker do describe "#perform" do let(:user) { FactoryBot.create(:user) } let(:user_id) { user.id } - let(:question) { FactoryBot.create(:question, user:) } + let(:content) { Faker::Lorem.sentence } + let(:question) { FactoryBot.create(:question, content:, user:) } let(:question_id) { question.id } before do @@ -92,5 +93,20 @@ describe QuestionWorker do ) end end + + context "long question" do + let(:content) { "x" * 1000 } + + it "sends to recipients who allow long questions" do + user.followers.first.profile.update(allow_long_questions: true) + + expect { subject } + .to( + change { Inbox.where(user_id: user.followers.ids, question_id:, new: true).count } + .from(0) + .to(1) + ) + end + end end end