From cefb805243585888fa966ecb0dee82c66b49fb09 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 02:50:10 +0200 Subject: [PATCH 1/8] Add `SubscriptionsController` --- app/controllers/subscriptions_controller.rb | 39 +++++++++++++++++++++ app/views/subscriptions/_create.html.haml | 3 ++ app/views/subscriptions/_destroy.html.haml | 3 ++ config/routes.rb | 2 ++ 4 files changed, 47 insertions(+) create mode 100644 app/controllers/subscriptions_controller.rb create mode 100644 app/views/subscriptions/_create.html.haml create mode 100644 app/views/subscriptions/_destroy.html.haml diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb new file mode 100644 index 00000000..388e72a6 --- /dev/null +++ b/app/controllers/subscriptions_controller.rb @@ -0,0 +1,39 @@ +class SubscriptionsController < ApplicationController + include TurboStreamable + + before_action :authenticate_user! + + turbo_stream_actions :create, :destroy + + def create + answer = Answer.find(params[:answer]) + result = Subscription.subscribe(current_user, answer) + + respond_to do |format| + format.turbo_stream do + render turbo_stream: [ + turbo_stream.replace("subscription-#{answer.id}", partial: "subscriptions/destroy", locals: { answer: }), + render_toast(t(result.present? ? ".success" : ".error"), result.present?) + ] + end + + format.html { redirect_to answer_path(username: answer.user.screen_name, id: answer.id) } + end + end + + def destroy + answer = Answer.find(params[:answer]) + result = Subscription.unsubscribe(current_user, answer) + + respond_to do |format| + format.turbo_stream do + render turbo_stream: [ + turbo_stream.replace("subscription-#{answer.id}", partial: "subscriptions/create", locals: { answer: }), + render_toast(t(result.present? ? ".success" : ".error"), result.present?) + ] + end + + format.html { redirect_to answer_path(username: answer.user.screen_name, id: answer.id) } + end + end +end diff --git a/app/views/subscriptions/_create.html.haml b/app/views/subscriptions/_create.html.haml new file mode 100644 index 00000000..d35861aa --- /dev/null +++ b/app/views/subscriptions/_create.html.haml @@ -0,0 +1,3 @@ += button_to subscriptions_path(answer: answer.id), class: "dropdown-item", id: "subscription-#{answer.id}" do + %i.fa.fa-fw.fa-bell + = t("voc.subscribe") diff --git a/app/views/subscriptions/_destroy.html.haml b/app/views/subscriptions/_destroy.html.haml new file mode 100644 index 00000000..89f57ede --- /dev/null +++ b/app/views/subscriptions/_destroy.html.haml @@ -0,0 +1,3 @@ += button_to subscriptions_path(answer: answer.id), method: :delete, class: "dropdown-item", id: "subscription-#{answer.id}" do + %i.fa.fa-fw.fa-bell-slash + = t("voc.unsubscribe") diff --git a/config/routes.rb b/config/routes.rb index e5fcf899..5004d57c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -148,6 +148,8 @@ Rails.application.routes.draw do post "/inbox/create", to: "inbox#create", as: :inbox_create get "/inbox", to: "inbox#show", as: :inbox + resource :subscriptions, controller: :subscriptions, only: %i[create destroy] + get "/user/:username", to: "user#show" get "/@:username", to: "user#show", as: :user get "/@:username/a/:id", to: "answer#show", as: :answer From 563f834287c1096329bee378b4a82d86194e7f6b Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 02:51:49 +0200 Subject: [PATCH 2/8] Remove `Ajax::SubscriptionController` --- app/controllers/ajax/subscription_controller.rb | 17 ----------------- config/routes.rb | 2 -- 2 files changed, 19 deletions(-) delete mode 100644 app/controllers/ajax/subscription_controller.rb diff --git a/app/controllers/ajax/subscription_controller.rb b/app/controllers/ajax/subscription_controller.rb deleted file mode 100644 index fd574e78..00000000 --- a/app/controllers/ajax/subscription_controller.rb +++ /dev/null @@ -1,17 +0,0 @@ -class Ajax::SubscriptionController < AjaxController - before_action :authenticate_user! - - def subscribe - params.require :answer - @response[:status] = :okay - result = Subscription.subscribe(current_user, Answer.find(params[:answer])) - @response[:success] = result.present? - end - - def unsubscribe - params.require :answer - @response[:status] = :okay - result = Subscription.unsubscribe(current_user, Answer.find(params[:answer])) - @response[:success] = result&.destroyed? || false - end -end diff --git a/config/routes.rb b/config/routes.rb index 5004d57c..faaabed1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -128,8 +128,6 @@ Rails.application.routes.draw do post "/create_list", to: "list#create", as: :create_list post "/destroy_list", to: "list#destroy", as: :destroy_list post "/list_membership", to: "list#membership", as: :list_membership - post "/subscribe", to: "subscription#subscribe", as: :subscribe_answer - post "/unsubscribe", to: "subscription#unsubscribe", as: :unsubscribe_answer get "/webpush/key", to: "web_push#key", as: :webpush_key post "/webpush/check", to: "web_push#check", as: :webpush_check post "/webpush", to: "web_push#subscribe", as: :webpush_subscribe From 42b5b6ccc2e796a4a9bfd236fd3e184b3bb29130 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 02:52:19 +0200 Subject: [PATCH 3/8] Remove TypeScript subscription functionality --- .../retrospring/features/answerbox/index.ts | 2 - .../features/answerbox/subscribe.ts | 44 ------------------- 2 files changed, 46 deletions(-) delete mode 100644 app/javascript/retrospring/features/answerbox/subscribe.ts diff --git a/app/javascript/retrospring/features/answerbox/index.ts b/app/javascript/retrospring/features/answerbox/index.ts index 61dc7edc..cf088f8b 100644 --- a/app/javascript/retrospring/features/answerbox/index.ts +++ b/app/javascript/retrospring/features/answerbox/index.ts @@ -3,11 +3,9 @@ import registerAnswerboxCommentEvents from './comment'; import { answerboxDestroyHandler } from './destroy'; import { answerboxReportHandler } from './report'; import { answerboxSmileHandler } from './smile'; -import { answerboxSubscribeHandler } from './subscribe'; export default (): void => { registerEvents([ - { type: 'click', target: '[data-action=ab-submarine]', handler: answerboxSubscribeHandler, global: true }, { type: 'click', target: '[data-action=ab-report]', handler: answerboxReportHandler, global: true }, { type: 'click', target: '[data-action=ab-destroy]', handler: answerboxDestroyHandler, global: true }, { type: 'click', target: '[name=ab-smile]', handler: answerboxSmileHandler, global: true } diff --git a/app/javascript/retrospring/features/answerbox/subscribe.ts b/app/javascript/retrospring/features/answerbox/subscribe.ts deleted file mode 100644 index 977d410e..00000000 --- a/app/javascript/retrospring/features/answerbox/subscribe.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { post } from '@rails/request.js'; - -import I18n from 'retrospring/i18n'; -import { showNotification, showErrorNotification } from 'utilities/notifications'; - -export function answerboxSubscribeHandler(event: Event): void { - const button = event.target as HTMLButtonElement; - const id = button.dataset.aId; - let torpedo = 0; - let targetUrl; - event.preventDefault(); - - if (button.dataset.torpedo === 'yes') { - torpedo = 1; - } - - if (torpedo) { - targetUrl = '/ajax/subscribe'; - } else { - targetUrl = '/ajax/unsubscribe'; - } - - post(targetUrl, { - body: { - answer: id - }, - contentType: 'application/json' - }) - .then(async response => { - const data = await response.json; - - if (data.success) { - button.dataset.torpedo = ["yes", "no"][torpedo]; - button.children[0].nextSibling.textContent = ' ' + (torpedo ? I18n.translate('voc.unsubscribe') : I18n.translate('voc.subscribe')); - showNotification(I18n.translate(`frontend.subscription.${torpedo ? 'subscribe' : 'unsubscribe'}`)); - } else { - showErrorNotification(I18n.translate(`frontend.subscription.fail.${torpedo ? 'subscribe' : 'unsubscribe'}`)); - } - }) - .catch(err => { - console.log(err); - showErrorNotification(I18n.translate('frontend.error.message')); - }); -} \ No newline at end of file From 88429982295c2c63acb096f34ec1c376a4753bd8 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 02:52:38 +0200 Subject: [PATCH 4/8] Render shared subscription actions in answerbox actions --- app/views/actions/_answer.html.haml | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/app/views/actions/_answer.html.haml b/app/views/actions/_answer.html.haml index e63e452b..78b81363 100644 --- a/app/views/actions/_answer.html.haml +++ b/app/views/actions/_answer.html.haml @@ -1,13 +1,8 @@ .dropdown-menu.dropdown-menu-end{ role: :menu } - if subscribed_answer_ids&.include?(answer.id) - -# fun joke should subscribe? - %a.dropdown-item{ href: "#", data: { a_id: answer.id, action: "ab-submarine", torpedo: "no" } } - %i.fa.fa-fw.fa-anchor - = t("voc.unsubscribe") + = render "subscriptions/destroy", answer: answer - else - %a.dropdown-item{ href: "#", data: { a_id: answer.id, action: "ab-submarine", torpedo: "yes" } } - %i.fa.fa-fw.fa-anchor - = t("voc.subscribe") + = render "subscriptions/create", answer: answer - if privileged? answer.user %a.dropdown-item.text-danger{ href: "#", data: { a_id: answer.id, action: "ab-destroy" } } %i.fa.fa-fw.fa-trash-o From 1bc7d040540340de973f2ed7954fba0da0f482a1 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 02:52:57 +0200 Subject: [PATCH 5/8] Move subscription locales into controller scope --- config/locales/controllers.en.yml | 7 +++++++ config/locales/frontend.en.yml | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/config/locales/controllers.en.yml b/config/locales/controllers.en.yml index b0347cc9..08b549dc 100644 --- a/config/locales/controllers.en.yml +++ b/config/locales/controllers.en.yml @@ -196,6 +196,13 @@ en: error: :errors.invalid_otp destroy: success: "Two factor authentication has been disabled for your account." + subscriptions: + create: + success: "Successfully subscribed." + error: "Failed to subscribe to answer." + destroy: + success: "Successfully unsubscribed." + error: "Failed to unsubscribe from answer." user: sessions: create: diff --git a/config/locales/frontend.en.yml b/config/locales/frontend.en.yml index 96228659..327e780b 100644 --- a/config/locales/frontend.en.yml +++ b/config/locales/frontend.en.yml @@ -6,12 +6,6 @@ en: error: title: "Uh-oh…" message: "An error occurred, a developer should check the console for details" - subscription: - subscribe: "Successfully subscribed." - unsubscribe: "Successfully unsubscribed." - fail: - subscribe: "Failed to subscribe to answer." - unsubscribe: "Failed to unsubscribe from answer." list: confirm: title: "Are you sure?" From 7fdf978be138a35ab0d691b75f57508b7eeca17b Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 02:59:27 +0200 Subject: [PATCH 6/8] Fix rubocop nits --- app/controllers/subscriptions_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index 388e72a6..87049981 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class SubscriptionsController < ApplicationController include TurboStreamable From 6e6cf5358bc11ebbcf9b4a1440ee53fb258c8431 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Wed, 25 Oct 2023 03:10:24 +0200 Subject: [PATCH 7/8] Add specs for `SubscriptionsController` These are mostly the old `AjaxController::SubscriptionController` specs, but adjusted for Turbo (and the proper action names) --- ...ec.rb => subscriptions_controller_spec.rb} | 63 +++---------------- 1 file changed, 8 insertions(+), 55 deletions(-) rename spec/controllers/{ajax/subscription_controller_spec.rb => subscriptions_controller_spec.rb} (66%) diff --git a/spec/controllers/ajax/subscription_controller_spec.rb b/spec/controllers/subscriptions_controller_spec.rb similarity index 66% rename from spec/controllers/ajax/subscription_controller_spec.rb rename to spec/controllers/subscriptions_controller_spec.rb index 23499211..6b9951d4 100644 --- a/spec/controllers/ajax/subscription_controller_spec.rb +++ b/spec/controllers/subscriptions_controller_spec.rb @@ -2,41 +2,33 @@ require "rails_helper" -describe Ajax::SubscriptionController, :ajax_controller, type: :controller do +describe SubscriptionsController, type: :controller do # need to use a different user here, as after a create the user owning the # answer is automatically subscribed to it let(:answer_user) { FactoryBot.create(:user) } let(:answer) { FactoryBot.create(:answer, user: answer_user) } + let(:user) { FactoryBot.create(:user) } - describe "#subscribe" do + describe "#create" do let(:params) do { - answer: answer_id + answer: answer_id, } end - subject { post(:subscribe, params: params) } + subject { post(:create, params:, format: :turbo_stream) } context "when user is signed in" do before(:each) { sign_in(user) } context "when answer exists" do let(:answer_id) { answer.id } - let(:expected_response) do - { - "success" => true, - "status" => "okay", - "message" => anything - } - end context "when subscription does not exist" do it "creates a subscription on the answer" do expect { subject }.to(change { answer.subscriptions.count }.by(1)) expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id, user.id].sort) end - - include_examples "returns the expected response" end context "when subscription already exists" do @@ -46,26 +38,15 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do expect { subject }.to(change { answer.subscriptions.count }.by(0)) expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id, user.id].sort) end - - include_examples "returns the expected response" end end context "when answer does not exist" do let(:answer_id) { "Bielefeld" } - let(:expected_response) do - { - "success" => false, - "status" => "not_found", - "message" => anything - } - end it "does not create a new subscription" do expect { subject }.not_to(change { Subscription.count }) end - - include_examples "returns the expected response" end end @@ -79,27 +60,20 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do end end - describe "#unsubscribe" do + describe "#destroy" do let(:params) do { - answer: answer_id + answer: answer_id, } end - subject { post(:unsubscribe, params: params) } + subject { delete(:destroy, params:, format: :turbo_stream) } context "when user is signed in" do before(:each) { sign_in(user) } context "when answer exists" do let(:answer_id) { answer.id } - let(:expected_response) do - { - "success" => true, - "status" => "okay", - "message" => anything - } - end context "when subscription exists" do before(:each) { Subscription.subscribe(user, answer) } @@ -108,43 +82,22 @@ describe Ajax::SubscriptionController, :ajax_controller, type: :controller do expect { subject }.to(change { answer.subscriptions.count }.by(-1)) expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id].sort) end - - include_examples "returns the expected response" end context "when subscription does not exist" do - let(:expected_response) do - { - "success" => false, - "status" => "okay", - "message" => anything - } - end - it "does not modify the answer's subscriptions" do expect { subject }.to(change { answer.subscriptions.count }.by(0)) expect(answer.subscriptions.map { |s| s.user.id }.sort).to eq([answer_user.id].sort) end - - include_examples "returns the expected response" end end context "when answer does not exist" do let(:answer_id) { "Bielefeld" } - let(:expected_response) do - { - "success" => false, - "status" => "not_found", - "message" => anything - } - end it "does not create a new subscription" do expect { subject }.not_to(change { Subscription.count }) end - - include_examples "returns the expected response" end end From 46cee3a19219846e7db9685fab296a68f7380ad5 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Thu, 26 Oct 2023 08:20:56 +0200 Subject: [PATCH 8/8] Fix Turbo Stream replace targeting the wrong element --- app/views/subscriptions/_create.html.haml | 2 +- app/views/subscriptions/_destroy.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/subscriptions/_create.html.haml b/app/views/subscriptions/_create.html.haml index d35861aa..64d289a9 100644 --- a/app/views/subscriptions/_create.html.haml +++ b/app/views/subscriptions/_create.html.haml @@ -1,3 +1,3 @@ -= button_to subscriptions_path(answer: answer.id), class: "dropdown-item", id: "subscription-#{answer.id}" do += button_to subscriptions_path(answer: answer.id), class: "dropdown-item", form: { id: "subscription-#{answer.id}" } do %i.fa.fa-fw.fa-bell = t("voc.subscribe") diff --git a/app/views/subscriptions/_destroy.html.haml b/app/views/subscriptions/_destroy.html.haml index 89f57ede..31e6d967 100644 --- a/app/views/subscriptions/_destroy.html.haml +++ b/app/views/subscriptions/_destroy.html.haml @@ -1,3 +1,3 @@ -= button_to subscriptions_path(answer: answer.id), method: :delete, class: "dropdown-item", id: "subscription-#{answer.id}" do += button_to subscriptions_path(answer: answer.id), method: :delete, class: "dropdown-item", form: { id: "subscription-#{answer.id}" } do %i.fa.fa-fw.fa-bell-slash = t("voc.unsubscribe")