From 08d82e725665b49ce4dfa10fe55004ff2df3bf05 Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Fri, 20 Aug 2021 21:36:02 +0700 Subject: [PATCH] Profile generalization: cleanup (#12947) * Remove hardcoded instances of education field * WIP Access display_email_on_profile via User Settings * Remove unused profile column * WIP Fix reference to bg_color_hex and text_color_hex * WIP fix issues revealed by systems specs * WIP fix issues revealed by services specs * WIP Fix failing tests * WIP Fix spec failures * wip * Move two attributes from controller to decorator * Update comment * Match user settings changes * More consistently use user.tag_line Even before the profile changes we sometimes used the user.summary attribute directly but used the user.tag_line method in other places. * Remove delegation, rename inline concern * Drop profile columns from users table * Remove duplicated work display from header * Update work profile field handling * Update DUS + spec * Delegate more carefully * Update delegation guard * Adapt for removed delegation * Undo accidental schema changes * Fix seeds * Remove accidentaly change * Fix User#processed_website_url * Update guard clause * Update profile card content * Add Organization#profile * Be more conservative with profile fields * Spec fixes round 1 * Fix typo * Update spec * Limit number of header fields and update card content * Decorate correct model * Update factory * Update schema.rb * Fix validation * How bad could this possibly be? * Pretty bad, nevermind * Remove obsolete code * Reset profile fields during test runs * Move profile fields back to before(:suite) * Spec fixes * Remove accidentally re-added files * More spec fixes * Specs * Change User#tag_keywords_for_search * More spec fixes * Add comment * Undo accidental schema changes * Attempt spec fix * Remove fix attempt * Fix e2e test * Update spec * Remove guard clause * Remove hardcoded instances of education field * WIP Access display_email_on_profile via User Settings * Remove unused profile column * WIP fix issues revealed by systems specs * WIP fix issues revealed by services specs * WIP Fix failing tests * WIP Fix spec failures * wip * Move two attributes from controller to decorator * Update comment * More consistently use user.tag_line Even before the profile changes we sometimes used the user.summary attribute directly but used the user.tag_line method in other places. * Remove education * Add comment * WIP * Clean up mostly_work_with * WIP * Update work profile field handling * More work-related changes * Remove settings_only from display_area enum * Remove quickfix from _metadata partial * Remove special attributes * Remove leftover spec * Retrieve location from profile, not user * Profile.special_attributes no longer exists * Update specs * More spec fixes * Update UsersController * Update UsersController and spec * Fix e2e seeds * Minor cleanup * More e2e seed fixes * Fix profile field CSV * Fix e2e seeds * Move one more attribute in e2e seeds * Remove duplicate line * Clear inputs before typing in them * Fix formatting issues * Profiles::Update -> Users::Update * Remove RegistrationsController#resolve_profile_field_issues * Fix schema.rb * More cleanup * Fix specs * Fix remaining spec Co-authored-by: Jacob Herrington --- app/assets/images/forem-background.svg | 4 +-- app/assets/stylesheets/views/comments.scss | 2 +- .../api/v0/organizations_controller.rb | 2 +- .../api/v0/readinglist_controller.rb | 1 + app/controllers/api/v0/users_controller.rb | 2 +- app/controllers/profiles_controller.rb | 2 +- app/controllers/registrations_controller.rb | 12 ------- app/controllers/users_controller.rb | 2 +- app/models/user.rb | 25 +++----------- app/services/users/suggest_recent.rb | 2 +- app/services/{profiles => users}/update.rb | 8 ++--- app/validators/profile_validator.rb | 6 ++-- app/views/users/_metadata.html.erb | 2 +- app/views/users/_profile.html.erb | 3 -- babel.config.js | 4 +-- ...064924_remove_profile_columns_from_user.rb | 33 +++++++++++++++++++ db/schema.rb | 25 -------------- docs/frontend/preact.md | 2 +- docs/installation/windows.md | 8 ++--- docs/self-hosting.md | 6 ++-- spec/models/navigation_link_spec.rb | 2 +- spec/requests/user/user_profile_spec.rb | 3 +- .../{profiles => users}/update_spec.rb | 2 +- 23 files changed, 66 insertions(+), 92 deletions(-) rename app/services/{profiles => users}/update.rb (92%) create mode 100644 db/migrate/20210309064924_remove_profile_columns_from_user.rb rename spec/services/{profiles => users}/update_spec.rb (98%) diff --git a/app/assets/images/forem-background.svg b/app/assets/images/forem-background.svg index de4181e66..795d995ad 100644 --- a/app/assets/images/forem-background.svg +++ b/app/assets/images/forem-background.svg @@ -1,5 +1,3 @@ - - - + diff --git a/app/assets/stylesheets/views/comments.scss b/app/assets/stylesheets/views/comments.scss index 2996cbea4..22e5d833b 100644 --- a/app/assets/stylesheets/views/comments.scss +++ b/app/assets/stylesheets/views/comments.scss @@ -273,5 +273,5 @@ .personal-template-button:not(.active), .moderator-template-button:not(.active) { - @extend .crayons-btn--outlined; + @extend .crayons-btn--outlined; } diff --git a/app/controllers/api/v0/organizations_controller.rb b/app/controllers/api/v0/organizations_controller.rb index a2e6cf8a9..1d14dd317 100644 --- a/app/controllers/api/v0/organizations_controller.rb +++ b/app/controllers/api/v0/organizations_controller.rb @@ -33,7 +33,7 @@ module Api num = [per_page, 1000].min page = params[:page] || 1 - @users = @organization.users.select(USERS_FOR_SERIALIZATION).page(page).per(num) + @users = @organization.users.joins(:profile).select(USERS_FOR_SERIALIZATION).page(page).per(num) end def listings diff --git a/app/controllers/api/v0/readinglist_controller.rb b/app/controllers/api/v0/readinglist_controller.rb index ece02ac28..67d3bb151 100644 --- a/app/controllers/api/v0/readinglist_controller.rb +++ b/app/controllers/api/v0/readinglist_controller.rb @@ -30,6 +30,7 @@ module Api .decorate @users_by_id = User + .joins(:profile) .select(UsersController::SHOW_ATTRIBUTES_FOR_SERIALIZATION) .find(articles.map(&:user_id)) .index_by(&:id) diff --git a/app/controllers/api/v0/users_controller.rb b/app/controllers/api/v0/users_controller.rb index 34f1853c7..4e9a47af3 100644 --- a/app/controllers/api/v0/users_controller.rb +++ b/app/controllers/api/v0/users_controller.rb @@ -10,7 +10,7 @@ module Api ].freeze def show - relation = User.select(SHOW_ATTRIBUTES_FOR_SERIALIZATION) + relation = User.joins(:profile).select(SHOW_ATTRIBUTES_FOR_SERIALIZATION) @user = if params[:id] == "by_username" relation.find_by!(username: params[:url]) diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 0a789d9ae..59b0f0999 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -5,7 +5,7 @@ class ProfilesController < ApplicationController ALLOWED_USERS_SETTING_PARAMS = %i[display_email_on_profile brand_color1 brand_color2].freeze def update - update_result = Profiles::Update.call(current_user, update_params) + update_result = Users::Update.call(current_user, update_params) if update_result.success? flash[:settings_notice] = "Your profile has been updated" redirect_to user_settings_path diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index c8fe7c1ea..6a2540128 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -14,8 +14,6 @@ class RegistrationsController < Devise::RegistrationsController def create authorize(params, policy_class: RegistrationPolicy) - resolve_profile_field_issues - unless recaptcha_verified? flash[:notice] = "You must complete the recaptcha ✅" return redirect_to new_user_registration_path(state: "email_signup") @@ -58,16 +56,6 @@ class RegistrationsController < Devise::RegistrationsController end end - def resolve_profile_field_issues - # Run this data update script when in a state of "first user" in the event - # that we are in a state where this was not already run. - # This is likely only temporarily needed. - return unless Settings::General.waiting_on_first_user - - csv = Rails.root.join("lib/data/dev_profile_fields.csv") - ProfileFields::ImportFromCsv.call(csv) - end - def check_allowed_email(resource) domain = resource.email.split("@").last allow_list = Settings::Authentication.allowed_registration_email_domains diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fcb5d4923..8d6cdb7d8 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -180,7 +180,7 @@ class UsersController < ApplicationController user_params.merge!(params[:user].permit(ALLOWED_USER_PARAMS)) end - update_result = Profiles::Update.call(current_user, user: user_params, profile: profile_params) + update_result = Users::Update.call(current_user, user: user_params, profile: profile_params) render_update_response(update_result.success?, update_result.errors_as_sentence) end diff --git a/app/models/user.rb b/app/models/user.rb index 4c2fb821c..2a514d9a9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -35,18 +35,13 @@ class User < ApplicationRecord youtube_url ].freeze - INACTIVE_PROFILE_COLUMNS = %w[ - bg_color_hex - text_color_hex - email_public - ].freeze - self.ignored_columns = PROFILE_COLUMNS - # NOTE: @citizen428 This is temporary code during profile migration and will - # be removed. - concerning :ProfileMigration do + # NOTE: we are using an inline module to keep profile related things together. + concerning :Profiles do included do + has_one :profile, dependent: :destroy + # NOTE: There are rare cases were we want to skip this callback, primarily # in tests. `skip_callback` modifies global state, which is not thread-safe # and can cause hard to track down bugs. We use an instance-level attribute @@ -55,14 +50,6 @@ class User < ApplicationRecord # All new users should automatically have a profile after_create_commit -> { Profile.create(user: self) }, unless: :_skip_creating_profile - - # Getters and setters for unmapped profile attributes - PROFILE_COLUMNS.each do |column| - next if INACTIVE_PROFILE_COLUMNS.include?(column) - next unless column.in?(Profile.attributes) - - delegate column, "#{column}=", to: :profile, allow_nil: true - end end end @@ -88,7 +75,6 @@ class User < ApplicationRecord acts_as_followable acts_as_follower - has_one :profile, dependent: :destroy has_one :notification_setting, class_name: "Users::NotificationSetting", dependent: :destroy has_one :setting, class_name: "Users::Setting", dependent: :destroy @@ -264,9 +250,6 @@ class User < ApplicationRecord before_destroy :unsubscribe_from_newsletters, prepend: true before_destroy :destroy_follows, prepend: true - # NOTE: @citizen428 Temporary while migrating to generalized profiles - after_save { |user| user.profile&.save if user.profile&.changed? } - after_create_commit :send_welcome_notification after_commit :subscribe_to_mailchimp_newsletter diff --git a/app/services/users/suggest_recent.rb b/app/services/users/suggest_recent.rb index 975c53525..887f5dbf2 100644 --- a/app/services/users/suggest_recent.rb +++ b/app/services/users/suggest_recent.rb @@ -52,7 +52,7 @@ module Users end def relation_as_array(relation, limit:) - relation = relation.select(attributes_to_select) if attributes_to_select.any? + relation = relation.joins(:profile).select(attributes_to_select) if attributes_to_select.any? relation.order(updated_at: :desc).limit(limit).to_a end diff --git a/app/services/profiles/update.rb b/app/services/users/update.rb similarity index 92% rename from app/services/profiles/update.rb rename to app/services/users/update.rb index f511b8e89..7adf29f13 100644 --- a/app/services/profiles/update.rb +++ b/app/services/users/update.rb @@ -1,6 +1,4 @@ -# TODO: This should probably become `Users::Update` as we're dealing with core -# user attributes, profile attributes and user settings. -module Profiles +module Users # Deals with updates that affect fields on +Profile+ and/or +User+ in a transparent way. class Update using HashAnyKey @@ -15,11 +13,11 @@ module Profiles # and/or user attributes to update. The corresponding has keys are +:user+ and # +:profile+ respectively. # @example - # Profiles::Update.call( + # Users::Update.call( # current_user, # profile: { website_url: "https://example.com" }, # ) - # @return [Profiles::Update] a class instance that can be used for success checks + # @return [Users::Update] a class instance that can be used for success checks def self.call(user, updated_attributes = {}) new(user, updated_attributes).call end diff --git a/app/validators/profile_validator.rb b/app/validators/profile_validator.rb index 1c9f76e58..064edcb01 100644 --- a/app/validators/profile_validator.rb +++ b/app/validators/profile_validator.rb @@ -11,9 +11,9 @@ class ProfileValidator < ActiveModel::Validator }.with_indifferent_access.freeze def validate(record) - # NOTE: @citizen428 The summary is a base profile field, which we add to all - # new Forem instances, so it should be safe to validate. The method itself - # also guards against the field's absence. + # NOTE: The summary is a base profile field, which we add to all new Forem + # instances, so it should be safe to validate. The method itself also guards + # against the field's absence. record.errors.add(:summary, "is too long") if summary_too_long?(record) ProfileField.all.each do |field| diff --git a/app/views/users/_metadata.html.erb b/app/views/users/_metadata.html.erb index 5083fd7d2..f65fb1d9a 100644 --- a/app/views/users/_metadata.html.erb +++ b/app/views/users/_metadata.html.erb @@ -6,7 +6,7 @@ <%= sanitized_sidebar title.titleize %>

- <%= sanitized_sidebar value %> + <%= sanitized_sidebar value %>

<% end %> diff --git a/app/views/users/_profile.html.erb b/app/views/users/_profile.html.erb index 5127afd6a..4ae8ff13c 100644 --- a/app/views/users/_profile.html.erb +++ b/app/views/users/_profile.html.erb @@ -78,9 +78,6 @@ <% ProfileFieldGroup.non_empty_groups.each do |group| %> - <% next if group.name == "Basic" # TODO: @citizen428 Remove this after user settings work (email field) %> - <% next if group.name == "Links" # TODO: [@jacobherrington] Remove this when we drop social links %> - <% next if group.name == "Branding" # TODO: [@msarit @jacobherrington] May be further refactored later as part of Profiles work %>

<%= group.name %>

<% if group.description.present? %> diff --git a/babel.config.js b/babel.config.js index 652dd7f97..2fc2f0f5a 100644 --- a/babel.config.js +++ b/babel.config.js @@ -55,13 +55,13 @@ module.exports = function (api) { '@babel/plugin-proposal-private-property-in-object', { loose: true, - } + }, ], [ '@babel/plugin-proposal-private-methods', { loose: true, - } + }, ], [ '@babel/plugin-transform-react-jsx', diff --git a/db/migrate/20210309064924_remove_profile_columns_from_user.rb b/db/migrate/20210309064924_remove_profile_columns_from_user.rb new file mode 100644 index 000000000..bab70e8dd --- /dev/null +++ b/db/migrate/20210309064924_remove_profile_columns_from_user.rb @@ -0,0 +1,33 @@ +class RemoveProfileColumnsFromUser < ActiveRecord::Migration[6.0] + def change + # rubocop:disable Metrics/Blocklength + safety_assured do + remove_column :users, :available_for + remove_column :users, :behance_url + remove_column :users, :bg_color_hex + remove_column :users, :currently_hacking_on + remove_column :users, :currently_learning + remove_column :users, :dribbble_url + remove_column :users, :education + remove_column :users, :email_public + remove_column :users, :employer_name + remove_column :users, :employer_url + remove_column :users, :employment_title + remove_column :users, :facebook_url + remove_column :users, :gitlab_url + remove_column :users, :instagram_url + remove_column :users, :linkedin_url + remove_column :users, :location + remove_column :users, :mastodon_url + remove_column :users, :medium_url + remove_column :users, :mostly_work_with + remove_column :users, :stackoverflow_url + remove_column :users, :summary + remove_column :users, :text_color_hex + remove_column :users, :twitch_url + remove_column :users, :website_url + remove_column :users, :youtube_url + end + # rubocop:enable Metrics/Blocklength + end +end diff --git a/db/schema.rb b/db/schema.rb index 355b86c63..fd6de9766 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1276,10 +1276,7 @@ ActiveRecord::Schema.define(version: 2021_07_20_042422) do create_table "users", force: :cascade do |t| t.string "apple_username" t.integer "articles_count", default: 0, null: false - t.string "available_for" t.integer "badge_achievements_count", default: 0, null: false - t.string "behance_url" - t.string "bg_color_hex" t.bigint "blocked_by_count", default: 0, null: false t.bigint "blocking_others_count", default: 0, null: false t.boolean "checked_code_of_conduct", default: false @@ -1292,19 +1289,10 @@ ActiveRecord::Schema.define(version: 2021_07_20_042422) do t.integer "credits_count", default: 0, null: false t.datetime "current_sign_in_at" t.inet "current_sign_in_ip" - t.string "currently_hacking_on" - t.string "currently_learning" - t.string "dribbble_url" - t.string "education" t.string "email" - t.boolean "email_public", default: false - t.string "employer_name" - t.string "employer_url" - t.string "employment_title" t.string "encrypted_password", default: "", null: false t.boolean "export_requested", default: false t.datetime "exported_at" - t.string "facebook_url" t.string "facebook_username" t.integer "failed_attempts", default: 0 t.datetime "feed_fetched_at", default: "2017-01-01 05:00:00" @@ -1313,8 +1301,6 @@ ActiveRecord::Schema.define(version: 2021_07_20_042422) do t.integer "following_users_count", default: 0, null: false t.datetime "github_repos_updated_at", default: "2017-01-01 05:00:00" t.string "github_username" - t.string "gitlab_url" - t.string "instagram_url" t.datetime "invitation_accepted_at" t.datetime "invitation_created_at" t.integer "invitation_limit" @@ -1333,13 +1319,8 @@ ActiveRecord::Schema.define(version: 2021_07_20_042422) do t.datetime "last_sign_in_at" t.inet "last_sign_in_ip" t.datetime "latest_article_updated_at" - t.string "linkedin_url" - t.string "location" t.datetime "locked_at" - t.string "mastodon_url" - t.string "medium_url" t.integer "monthly_dues", default: 0 - t.string "mostly_work_with" t.string "name" t.string "old_old_username" t.string "old_username" @@ -1363,21 +1344,15 @@ ActiveRecord::Schema.define(version: 2021_07_20_042422) do t.integer "sign_in_count", default: 0, null: false t.string "signup_cta_variant" t.integer "spent_credits_count", default: 0, null: false - t.string "stackoverflow_url" t.string "stripe_id_code" t.integer "subscribed_to_user_subscriptions_count", default: 0, null: false - t.text "summary" - t.string "text_color_hex" - t.string "twitch_url" t.string "twitter_username" t.string "unconfirmed_email" t.string "unlock_token" t.integer "unspent_credits_count", default: 0, null: false t.datetime "updated_at", null: false t.string "username" - t.string "website_url" t.datetime "workshop_expiration" - t.string "youtube_url" t.index "to_tsvector('simple'::regconfig, COALESCE((name)::text, ''::text))", name: "index_users_on_name_as_tsvector", using: :gin t.index "to_tsvector('simple'::regconfig, COALESCE((username)::text, ''::text))", name: "index_users_on_username_as_tsvector", using: :gin t.index ["apple_username"], name: "index_users_on_apple_username" diff --git a/docs/frontend/preact.md b/docs/frontend/preact.md index 74daa2c16..288c242f2 100644 --- a/docs/frontend/preact.md +++ b/docs/frontend/preact.md @@ -56,7 +56,7 @@ import { tagPropTypes } from '../../../components/common-prop-types'; const SomeComponentUsingTags = ({ tags = [] }) => (
    - {tags.map(tag => ( + {tags.map((tag) => (
  • {tag.name}
    • ))}
diff --git a/docs/installation/windows.md b/docs/installation/windows.md index 91f4f8bea..f0fda0702 100644 --- a/docs/installation/windows.md +++ b/docs/installation/windows.md @@ -290,8 +290,8 @@ There are currently two work-arounds. ### WSL2 and End to End Tests -In order to run Cypress Test Runner, a few extra setup steps are required to run GUIs -natively inside of WSL2. +In order to run Cypress Test Runner, a few extra setup steps are required to run +GUIs natively inside of WSL2. -[This guide](https://nickymeuleman.netlify.app/blog/gui-on-wsl2-cypress) explains -how to use graphical user interfaces like Cypress inside WSL2. +[This guide](https://nickymeuleman.netlify.app/blog/gui-on-wsl2-cypress) +explains how to use graphical user interfaces like Cypress inside WSL2. diff --git a/docs/self-hosting.md b/docs/self-hosting.md index f4269c560..62700e6a0 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -4,6 +4,8 @@ title: Self Hosting Forem ## Hosting your own Forem instance -Visit [the official Forem Self-Host playbook](https://github.com/forem/selfhost) in order to host on DigitalOcean, AWS, Google Cloud, or bare metal. +Visit [the official Forem Self-Host playbook](https://github.com/forem/selfhost) +in order to host on DigitalOcean, AWS, Google Cloud, or bare metal. -If you intend to stand up a Forem, we _strongly_ encourage you to join [forem.dev](https://forem.dev) to take part and keep up with changes. 🌱 +If you intend to stand up a Forem, we _strongly_ encourage you to join +[forem.dev](https://forem.dev) to take part and keep up with changes. 🌱 diff --git a/spec/models/navigation_link_spec.rb b/spec/models/navigation_link_spec.rb index e56c73b95..9cd815ac9 100644 --- a/spec/models/navigation_link_spec.rb +++ b/spec/models/navigation_link_spec.rb @@ -13,7 +13,7 @@ RSpec.describe NavigationLink, type: :model do it "validates the icon" do navigation_link.icon = "test.png" expect(navigation_link).not_to be_valid - + navigation_link.icon = "" expect(navigation_link).to be_valid diff --git a/spec/requests/user/user_profile_spec.rb b/spec/requests/user/user_profile_spec.rb index 4540b0d1d..d340f766d 100644 --- a/spec/requests/user/user_profile_spec.rb +++ b/spec/requests/user/user_profile_spec.rb @@ -99,8 +99,7 @@ RSpec.describe "UserProfiles", type: :request do end it "does not render special display header elements naively" do - user.profile.location = "hawaii" - user.save + user.profile.update(location: "hawaii") get "/#{user.username}" # Does not include the word, but does include the SVG expect(response.body).not_to include "

Location

" diff --git a/spec/services/profiles/update_spec.rb b/spec/services/users/update_spec.rb similarity index 98% rename from spec/services/profiles/update_spec.rb rename to spec/services/users/update_spec.rb index 95f86d351..618846275 100644 --- a/spec/services/profiles/update_spec.rb +++ b/spec/services/users/update_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe Profiles::Update, type: :service do +RSpec.describe Users::Update, type: :service do def sidekiq_assert_resave_article_worker(user, &block) sidekiq_assert_enqueued_with( job: Users::ResaveArticlesWorker,