Refactoring onboarding (#19525)

* Quick refactor onboardings controller(s)

* Quick refactor onboardings controller(s), pt2

* Quick refactor onboardings controller(s), pt3

* Quick refactor onboardings controller(s), pt4

* Quick refactor onboardings controller(s), pt5

* Add test for checkboxes
This commit is contained in:
Joshua Wehner 2023-05-25 10:03:12 +02:00 committed by GitHub
parent 772f6cdd7b
commit 862cd54dc7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 291 additions and 278 deletions

View file

@ -45,7 +45,7 @@ class ApplicationController < ActionController::Base
private_constant :PUBLIC_CONTROLLERS
CONTENT_CHANGE_PATHS = [
"/tags/onboarding", # Needs to change when suggested_tags is edited.
"/onboarding/tags", # Needs to change when suggested_tags is edited.
"/onboarding", # Page is cached at edge.
"/", # Page is cached at edge.
].freeze

View file

@ -0,0 +1,98 @@
class OnboardingsController < ApplicationController
before_action :authenticate_user!
before_action :check_suspended, only: %i[notifications]
before_action :set_cache_control_headers, only: %i[show tags]
before_action :set_no_cache_header, only: %i[update]
after_action :verify_authorized, only: %i[update checkbox]
TAG_ONBOARDING_ATTRIBUTES = %i[id name taggings_count].freeze
ALLOWED_USER_PARAMS = %i[last_onboarding_page username].freeze
ALLOWED_CHECKBOX_PARAMS = %i[checked_code_of_conduct checked_terms_and_conditions].freeze
ALLOWED_NOTIFICATION_PARAMS = %i[email_newsletter email_digest_periodic].freeze
def show
set_surrogate_key_header "onboarding-slideshow"
end
def tags
@tags = Tags::SuggestedForOnboarding.call
.select(TAG_ONBOARDING_ATTRIBUTES)
render json: @tags
set_surrogate_key_header Tag.table_key, @tags.map(&:record_key)
end
def update
authorize User, :onboarding_update?
user_params = {}
if params[:user]
if unset_username?
return render json: { errors: I18n.t("users_controller.username_blank") }, status: :unprocessable_entity
end
sanitize_user_params
user_params = params[:user].permit(ALLOWED_USER_PARAMS)
end
update_result = Users::Update.call(current_user, user: user_params, profile: profile_params)
if update_result.success?
render json: {}, status: :ok
else
render json: { errors: update_result.errors_as_sentence }, status: :unprocessable_entity
end
end
def checkbox
authorize User, :onboarding_checkbox_update?
if params[:user]
current_user.assign_attributes(params[:user].permit(ALLOWED_CHECKBOX_PARAMS))
end
current_user.saw_onboarding = true
if current_user.save
render json: {}, status: :ok
else
render json: { errors: errors }, status: :unprocessable_entity
end
end
def notifications
authorize User, :onboarding_notifications_checkbox_update?
if params[:notifications]
current_user.notification_setting.assign_attributes(params[:notifications].permit(ALLOWED_NOTIFICATION_PARAMS))
end
current_user.saw_onboarding = true
success = current_user.notification_setting.save
notifications_updated_response(success, current_user.notification_setting.errors_as_sentence)
end
private
def unset_username?
params[:user].key?(:username) && params[:user][:username].blank?
end
def sanitize_user_params
params[:user].compact_blank!
end
def profile_params
params[:profile]&.permit(Profile.static_fields + Profile.attributes)
end
def notifications_updated_response(success, errors)
status = success ? 200 : 422
respond_to do |format|
format.json { render json: { errors: errors }, status: status }
end
end
end

View file

@ -1,10 +1,8 @@
class TagsController < ApplicationController
before_action :set_cache_control_headers, only: %i[index onboarding]
before_action :set_cache_control_headers, only: %i[index]
before_action :authenticate_user!, only: %i[edit update]
after_action :verify_authorized
ATTRIBUTES_FOR_SERIALIZATION = %i[id name bg_color_hex text_color_hex short_summary badge_id].freeze
ONBOARDING_API_ATTRIBUTES = %i[id name taggings_count].freeze
INDEX_API_ATTRIBUTES = %i[name rules_html short_summary bg_color_hex badge_id].freeze
TAGS_ALLOWED_PARAMS = %i[
@ -24,7 +22,7 @@ class TagsController < ApplicationController
def bulk
skip_authorization
@tags = Tag.includes(:badge).select(ATTRIBUTES_FOR_SERIALIZATION)
@tags = Tag.includes(:badge).select_attributes_for_serialization
page = params[:page]
per_page = (params[:per_page] || 10).to_i
@ -36,8 +34,8 @@ class TagsController < ApplicationController
@tags = @tags.where(name: params[:tag_names])
end
@tags = @tags.order(taggings_count: :desc).page(page).per(num)
render json: @tags, only: ATTRIBUTES_FOR_SERIALIZATION, include: [badge: { only: [:badge_image] }]
@tags = @tags.order(taggings_count: :desc).select_attributes_for_serialization.page(page).per(num)
render json: @tags, only: Tag::ATTRIBUTES_FOR_SERIALIZATION, include: [badge: { only: [:badge_image] }]
end
def edit
@ -63,16 +61,6 @@ class TagsController < ApplicationController
redirect_to edit_admin_tag_path(tag.id)
end
def onboarding
skip_authorization
@tags = Tags::SuggestedForOnboarding.call
.select(ONBOARDING_API_ATTRIBUTES)
render json: @tags
set_surrogate_key_header Tag.table_key, @tags.map(&:record_key)
end
def suggest
skip_authorization
tags = Tag.supported.order(hotness_score: :desc).limit(100).select(INDEX_API_ATTRIBUTES)
@ -95,6 +83,4 @@ class TagsController < ApplicationController
convert_empty_string_to_nil
params.require(:tag).permit(TAGS_ALLOWED_PARAMS)
end
private_constant :ATTRIBUTES_FOR_SERIALIZATION
end

View file

@ -34,18 +34,6 @@ module Users
redirect_to user_settings_path(:notifications)
end
def onboarding_notifications_checkbox_update
authorize User
if params[:notifications]
current_user.notification_setting.assign_attributes(params[:notifications].permit(ONBOARDING_ALLOWED_PARAMS))
end
current_user.saw_onboarding = true
success = current_user.notification_setting.save
render_update_response(success, current_user.notification_setting.errors_as_sentence)
end
private
def render_update_response(success, errors = nil)

View file

@ -1,67 +0,0 @@
module Users
class OnboardingsController < ApplicationController
before_action :authenticate_user!
before_action :set_cache_control_headers, only: [:show]
before_action :set_no_cache_header, only: %i[update onboarding_checkbox_update]
after_action :verify_authorized, except: [:show]
ALLOWED_USER_PARAMS = %i[last_onboarding_page username].freeze
ALLOWED_CHECKBOX_PARAMS = %i[checked_code_of_conduct checked_terms_and_conditions].freeze
def show
set_surrogate_key_header "onboarding-slideshow"
end
def update
authorize User, :onboarding_update?
user_params = {}
if params[:user]
if unset_username?
return render json: { errors: I18n.t("users_controller.username_blank") }, status: :unprocessable_entity
end
sanitize_user_params
user_params = params[:user].permit(ALLOWED_USER_PARAMS)
end
update_result = Users::Update.call(current_user, user: user_params, profile: profile_params)
if update_result.success?
render json: {}, status: :ok
else
render json: { errors: update_result.errors_as_sentence }, status: :unprocessable_entity
end
end
def onboarding_checkbox_update
if params[:user]
current_user.assign_attributes(params[:user].permit(ALLOWED_CHECKBOX_PARAMS))
end
current_user.saw_onboarding = true
authorize User
if current_user.save
render json: {}, status: :ok
else
render json: { errors: errors }, status: :unprocessable_entity
end
end
private
def unset_username?
params[:user].key?(:username) && params[:user][:username].blank?
end
def sanitize_user_params
params[:user].compact_blank!
end
def profile_params
params[:profile]&.permit(Profile.static_fields + Profile.attributes)
end
end
end

View file

@ -24,7 +24,7 @@ export class EmailPreferencesForm extends Component {
onSubmit() {
const csrfToken = getContentOfToken('csrf-token');
fetch('/onboarding_notifications_checkbox_update', {
fetch('/onboarding/notifications', {
method: 'PATCH',
headers: {
'X-CSRF-Token': csrfToken,

View file

@ -18,7 +18,7 @@ export class FollowTags extends Component {
}
componentDidMount() {
fetch('/tags/onboarding')
fetch('/onboarding/tags')
.then((response) => response.json())
.then((data) => {
this.setState({ allTags: data });

View file

@ -28,7 +28,7 @@ export class IntroSlide extends Component {
const { next } = this.props;
const csrfToken = getContentOfToken('csrf-token');
fetch('/onboarding_checkbox_update', {
fetch('/onboarding/checkbox', {
method: 'PATCH',
headers: {
'X-CSRF-Token': csrfToken,

View file

@ -33,6 +33,7 @@ class Tag < ActsAsTaggableOn::Tag
include StringAttributeCleaner.nullify_blanks_for(:alias_for)
ALLOWED_CATEGORIES = %w[uncategorized language library tool site_mechanic location subcommunity].freeze
HEX_COLOR_REGEXP = /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/
ATTRIBUTES_FOR_SERIALIZATION = %i[id name bg_color_hex text_color_hex short_summary badge_id].freeze
belongs_to :badge, optional: true
@ -75,6 +76,8 @@ class Tag < ActsAsTaggableOn::Tag
# this scope we have a name.
scope :direct, -> { where(alias_for: [nil, ""]) }
scope :select_attributes_for_serialization, -> { select(ATTRIBUTES_FOR_SERIALIZATION) }
pg_search_scope :search_by_name,
against: :name,
using: { tsearch: { prefix: true } }

View file

@ -39,7 +39,7 @@
<%# BEGIN Feed menu bar %>
<main class="articles-list crayons-layout__content" id="main-content" data-follow-button-container="true">
<%= render(partial: "users/onboardings/task_card") if user_signed_in? %>
<%= render(partial: "onboardings/task_card") if user_signed_in? %>
<header class="p-2 px-3 m:p-0 m:px-0 m:mb-2 fs-l">
<h1 class="screen-reader-only"><%= t("views.stories.heading") %></h1>

View file

@ -1,4 +0,0 @@
json.array! @tags.each do |tag|
json.extract!(tag, :id, :name, :bg_color_hex, :text_color_hex)
json.following nil
end

View file

@ -114,7 +114,6 @@ Rails.application.routes.draw do
resources :notifications, only: [:index]
resources :tags, only: [:index] do
collection do
get "/onboarding", to: "tags#onboarding"
get "/suggest", to: "tags#suggest", defaults: { format: :json }
get "/bulk", to: "tags#bulk", defaults: { format: :json }
end
@ -150,6 +149,7 @@ Rails.application.routes.draw do
get "/subscribed", action: "subscribed"
end
end
namespace :followings, defaults: { format: :json } do
get :users
get :tags
@ -157,9 +157,12 @@ Rails.application.routes.draw do
get :podcasts
end
scope module: "users" do
resource :onboarding, only: %i[show update]
patch "/onboarding_checkbox_update", to: "onboardings#onboarding_checkbox_update"
resource :onboarding, only: %i[show update] do
member do
patch :checkbox
patch :notifications
get :tags
end
end
resources :profiles, only: %i[update]
@ -178,8 +181,6 @@ Rails.application.routes.draw do
get "/notifications/:filter/:org_id", to: "notifications#index", as: :notifications_filter_org
get "/notification_subscriptions/:notifiable_type/:notifiable_id", to: "notification_subscriptions#show"
post "/notification_subscriptions/:notifiable_type/:notifiable_id", to: "notification_subscriptions#upsert"
patch "/onboarding_notifications_checkbox_update",
to: "users/notification_settings#onboarding_notifications_checkbox_update"
get "email_subscriptions/unsubscribe"
get "/internal", to: redirect("/admin")

View file

@ -1,7 +1,12 @@
require "rails_helper"
RSpec.describe "Onboardings" do
let(:user) { create(:user, saw_onboarding: false) }
let(:user) do
create(:user,
saw_onboarding: false,
_skip_creating_profile: true,
profile: create(:profile, location: "Llama Town"))
end
describe "GET /onboarding" do
it "redirects user if unauthenticated" do
@ -34,4 +39,172 @@ RSpec.describe "Onboardings" do
expect(response.body).to include(Settings::General.onboarding_background_image)
end
end
describe "GET /onboarding/tags" do
let(:headers) do
{
Accept: "application/json",
"Content-Type": "application/json"
}
end
before do
sign_in user
allow(Settings::General).to receive(:suggested_tags).and_return(%w[beginners javascript career])
end
it "returns tags" do
create(:tag, name: Settings::General.suggested_tags.first)
get tags_onboarding_path, headers: headers
expect(response.parsed_body.size).to eq(1)
end
it "returns tags with the correct json representation" do
tag = create(:tag, name: Settings::General.suggested_tags.first)
get tags_onboarding_path, headers: headers
response_tag = response.parsed_body.first
expect(response_tag.keys).to \
match_array(OnboardingsController::TAG_ONBOARDING_ATTRIBUTES.map(&:to_s))
expect(response_tag["id"]).to eq(tag.id)
expect(response_tag["name"]).to eq(tag.name)
expect(response_tag["taggings_count"]).to eq(tag.taggings_count)
end
it "returns suggested and supported tags" do
not_suggested_but_supported = create(:tag, name: "notsuggestedbutsupported", supported: true, suggested: false)
neither_suggested_nor_supported = create(:tag, name: "definitelynotasuggestedtag", supported: false)
get tags_onboarding_path, headers: headers
expect(response.parsed_body.filter { |t| t["name"] == not_suggested_but_supported.name }).not_to be_empty
expect(response.parsed_body.filter { |t| t["name"] == neither_suggested_nor_supported.name }).to be_empty
end
it "sets the correct edge caching surrogate key for all tags" do
tag = create(:tag, name: Settings::General.suggested_tags.first)
get tags_onboarding_path, headers: headers
expected_key = ["tags", tag.record_key].to_set
expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
end
end
describe "PATCH /onboarding" do
context "when signed in" do
before { sign_in user }
it "updates the user's last_onboarding_page attribute" do
params = { user: { last_onboarding_page: "v2: personal info form", username: "test" } }
expect do
patch "/onboarding", params: params
end.to change(user, :last_onboarding_page)
end
it "updates the user's username attribute" do
params = { user: { username: "WilhuffTarkin" } }
expect do
patch "/onboarding", params: params
end.to change(user, :username).to("wilhufftarkin")
end
it "returns a 422 error if the username is blank" do
params = { user: { username: "" } }
patch "/onboarding", params: params
expect(response).to have_http_status(:unprocessable_entity)
end
it "updates the user's profile" do
params = { profile: { location: "Galactic Empire" } }
expect do
patch "/onboarding", params: params
end.to change(user.profile, :location).to("Galactic Empire")
end
it "does not update the user's last_onboarding_page if it is empty" do
params = { user: { last_onboarding_page: "" } }
expect do
patch "/onboarding", params: params
end.not_to change(user, :last_onboarding_page)
end
end
context "when signed out" do
it "returns a not found error if user is not signed in" do
patch "/onboarding.json", params: {}
expect(response.parsed_body["error"]).to include("Please sign in")
end
end
end
describe "PATCH /onboarding/checkbox" do
context "when signed in" do
before { sign_in user }
it "updates saw_onboarding boolean" do
patch "/onboarding/checkbox.json", params: {}
expect(user.saw_onboarding).to be(true)
end
it "updates checked_code_of_conduct and checked_terms_and_conditions" do
patch "/onboarding/checkbox.json",
params: {
checked_code_of_conduct: "1",
checked_terms_and_conditions: "1"
}
expect(user.checked_code_of_conduct).to be(true)
expect(user.checked_terms_and_conditions).to be(true)
end
end
context "when signed out" do
it "returns a not found error if user is not signed in" do
patch "/onboarding/checkbox.json", params: {}
expect(response.parsed_body["error"]).to include("Please sign in")
end
end
end
describe "PATCH /onboarding/notifications" do
before { sign_in user }
it "updates onboarding checkbox" do
user.update_column(:saw_onboarding, false)
expect do
patch notifications_onboarding_path(format: :json),
params: { notifications: { tab: "notifications", email_newsletter: 1 } }
end.to change { user.notification_setting.reload.email_newsletter }.from(false).to(true)
expect(user.saw_onboarding).to be(true)
end
it "can toggle email_newsletter" do
expect do
patch notifications_onboarding_path(format: :json),
params: { notifications: { tab: "notifications", email_newsletter: 1 } }
end.to change { user.notification_setting.reload.email_newsletter }.from(false).to(true)
expect do
patch notifications_onboarding_path(format: :json),
params: { notifications: { tab: "notifications", email_newsletter: 0 } }
end.to change { user.notification_setting.reload.email_newsletter }.from(true).to(false)
end
it "can toggle email_digest_periodic" do
expect do
patch notifications_onboarding_path(format: :json),
params: { notifications: { tab: "notifications", email_digest_periodic: 1 } }
end.to change { user.notification_setting.reload.email_digest_periodic }.from(false).to(true)
expect do
patch notifications_onboarding_path(format: :json),
params: { notifications: { tab: "notifications", email_digest_periodic: 0 } }
end.to change { user.notification_setting.reload.email_digest_periodic }.from(true).to(false)
end
end
end

View file

@ -187,56 +187,4 @@ RSpec.describe "Tags", proper_status: true do
end
end
end
describe "GET /tags/onboarding" do
let(:headers) do
{
Accept: "application/json",
"Content-Type": "application/json"
}
end
before do
allow(Settings::General).to receive(:suggested_tags).and_return(%w[beginners javascript career])
end
it "returns tags" do
create(:tag, name: Settings::General.suggested_tags.first)
get onboarding_tags_path, headers: headers
expect(response.parsed_body.size).to eq(1)
end
it "returns tags with the correct json representation" do
tag = create(:tag, name: Settings::General.suggested_tags.first)
get onboarding_tags_path, headers: headers
response_tag = response.parsed_body.first
expect(response_tag.keys).to match_array(%w[id name taggings_count])
expect(response_tag["id"]).to eq(tag.id)
expect(response_tag["name"]).to eq(tag.name)
expect(response_tag["taggings_count"]).to eq(tag.taggings_count)
end
it "returns suggested and supported tags" do
not_suggested_but_supported = create(:tag, name: "notsuggestedbutsupported", supported: true, suggested: false)
neither_suggested_nor_supported = create(:tag, name: "definitelynotasuggestedtag", supported: false)
get onboarding_tags_path, headers: headers
expect(response.parsed_body.filter { |t| t["name"] == not_suggested_but_supported.name }).not_to be_empty
expect(response.parsed_body.filter { |t| t["name"] == neither_suggested_nor_supported.name }).to be_empty
end
it "sets the correct edge caching surrogate key for all tags" do
tag = create(:tag, name: Settings::General.suggested_tags.first)
get onboarding_tags_path, headers: headers
expected_key = ["tags", tag.record_key].to_set
expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
end
end
end

View file

@ -37,42 +37,4 @@ RSpec.describe "UserNotificationSettings" do
expect(user.notification_setting.reload.subscribed_to_welcome_notifications?).to be(true)
end
end
describe "PATCH /onboarding_notifications_checkbox_update" do
before { sign_in user }
it "updates onboarding checkbox" do
user.update_column(:saw_onboarding, false)
expect do
patch onboarding_notifications_checkbox_update_path(format: :json),
params: { notifications: { tab: "notifications", email_newsletter: 1 } }
end.to change { user.notification_setting.reload.email_newsletter }.from(false).to(true)
expect(user.saw_onboarding).to be(true)
end
it "can toggle email_newsletter" do
expect do
patch onboarding_notifications_checkbox_update_path(format: :json),
params: { notifications: { tab: "notifications", email_newsletter: 1 } }
end.to change { user.notification_setting.reload.email_newsletter }.from(false).to(true)
expect do
patch onboarding_notifications_checkbox_update_path(format: :json),
params: { notifications: { tab: "notifications", email_newsletter: 0 } }
end.to change { user.notification_setting.reload.email_newsletter }.from(true).to(false)
end
it "can toggle email_digest_periodic" do
expect do
patch onboarding_notifications_checkbox_update_path(format: :json),
params: { notifications: { tab: "notifications", email_digest_periodic: 1 } }
end.to change { user.notification_setting.reload.email_digest_periodic }.from(false).to(true)
expect do
patch onboarding_notifications_checkbox_update_path(format: :json),
params: { notifications: { tab: "notifications", email_digest_periodic: 0 } }
end.to change { user.notification_setting.reload.email_digest_periodic }.from(true).to(false)
end
end
end

View file

@ -1,75 +0,0 @@
require "rails_helper"
RSpec.describe "UsersOnboarding" do
let!(:user) do
create(:user,
saw_onboarding: false,
_skip_creating_profile: true,
profile: create(:profile, location: "Llama Town"))
end
describe "PATCH /onboarding" do
context "when signed in" do
before { sign_in user }
it "updates the user's last_onboarding_page attribute" do
params = { user: { last_onboarding_page: "v2: personal info form", username: "test" } }
expect do
patch "/onboarding", params: params
end.to change(user, :last_onboarding_page)
end
it "updates the user's username attribute" do
params = { user: { username: "WilhuffTarkin" } }
expect do
patch "/onboarding", params: params
end.to change(user, :username).to("wilhufftarkin")
end
it "returns a 422 error if the username is blank" do
params = { user: { username: "" } }
patch "/onboarding", params: params
expect(response).to have_http_status(:unprocessable_entity)
end
it "updates the user's profile" do
params = { profile: { location: "Galactic Empire" } }
expect do
patch "/onboarding", params: params
end.to change(user.profile, :location).to("Galactic Empire")
end
it "does not update the user's last_onboarding_page if it is empty" do
params = { user: { last_onboarding_page: "" } }
expect do
patch "/onboarding", params: params
end.not_to change(user, :last_onboarding_page)
end
end
context "when signed out" do
it "returns a not found error if user is not signed in" do
patch "/onboarding.json", params: {}
expect(response.parsed_body["error"]).to include("Please sign in")
end
end
end
describe "PATCH /onboarding_checkbox_update" do
context "when signed in" do
before { sign_in user }
it "updates saw_onboarding boolean" do
patch "/onboarding_checkbox_update.json", params: {}
expect(user.saw_onboarding).to be(true)
end
end
context "when signed out" do
it "returns a not found error if user is not signed in" do
patch "/onboarding_checkbox_update.json", params: {}
expect(response.parsed_body["error"]).to include("Please sign in")
end
end
end
end