Profile generalization cleanup phase 2 (#14555)

* Remove ignored profile columns

* Update test.rb

* Remove dev profile fields CSV

* Fix first round of specs

* Add additional .includes

* More spec fixes

* Preload user in more locations

* Fix typo

* Spec/preloading fixes

* Specy McSpecFace

* Update e2e seeds

* Update e2e sees with correct display area

* Remove unused eager loading

* Update test.rb

* Add another entry to Bullet.safe_list

* Fix e2e seeds
This commit is contained in:
Michael Kohl 2021-08-27 09:17:26 +07:00 committed by GitHub
parent cb1a261a07
commit 1b87cc7626
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
26 changed files with 75 additions and 144 deletions

View file

@ -1,3 +1,3 @@
<svg width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg">
<path d="M5.868 2.75L8 10h8l2.132-7.25a.4.4 0 0 1 .765-.01l3.495 10.924a.5.5 0 0 1-.173.55L12 22 1.78 14.214a.5.5 0 0 1-.172-.55L5.103 2.74a.4.4 0 0 1 .765.009v.001z"/>
<path d="M5.868 2.75 8 10h8l2.132-7.25a.4.4 0 0 1 .765-.01l3.495 10.924a.5.5 0 0 1-.173.55L12 22 1.78 14.214a.5.5 0 0 1-.172-.55L5.103 2.74a.4.4 0 0 1 .765.009v.001z"/>
</svg>

Before

Width:  |  Height:  |  Size: 264 B

After

Width:  |  Height:  |  Size: 264 B

View file

@ -1,6 +1,6 @@
<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M12 20.899a9.444 9.444 0 0 0 9.444-9.445 9.444 9.444 0 0 0-18.888 0A9.444 9.444 0 0 0 12 20.9z" fill="#FFCB4C"/>
<path d="M10.069 13.687a.508.508 0 0 1-.173-.258.444.444 0 0 1 .327-.567c2.516-.568 4.21.757 4.281.814.214.17.255.469.097.669-.16.199-.46.223-.673.054-.061-.047-1.402-1.068-3.435-.61a.506.506 0 0 1-.424-.102zM9.288 9.683c.652 0 1.18-.66 1.18-1.475 0-.815-.528-1.476-1.18-1.476-.652 0-1.18.66-1.18 1.476 0 .815.528 1.475 1.18 1.475zM15.542 10.273c.652 0 1.18-.66 1.18-1.475 0-.815-.528-1.476-1.18-1.476-.652 0-1.18.66-1.18 1.476 0 .815.528 1.475 1.18 1.475z" fill="#65471B"/>
<path d="M10.069 13.687a.508.508 0 0 1-.173-.258.444.444 0 0 1 .327-.567c2.516-.568 4.21.757 4.281.814.214.17.255.469.097.669-.16.199-.46.223-.673.054-.061-.047-1.402-1.068-3.435-.61a.506.506 0 0 1-.424-.102zm-.781-4.004c.652 0 1.18-.66 1.18-1.475 0-.815-.528-1.476-1.18-1.476-.652 0-1.18.66-1.18 1.476 0 .815.528 1.475 1.18 1.475zm6.254.59c.652 0 1.18-.66 1.18-1.475 0-.815-.528-1.476-1.18-1.476-.652 0-1.18.66-1.18 1.476 0 .815.528 1.475 1.18 1.475z" fill="#65471B"/>
<path d="M11.598 21.527s.703-.228.794-.75c.096-.54-.347-.65-.347-.65s.578-.115.651-.764c.068-.611-.478-.757-.478-.757s.539-.222.564-.855c.021-.533-.553-.793-.553-.793s2.8-.679 3.087-.745c.287-.067.733-.342.594-.941-.138-.6-.669-.621-.943-.558-.274.064-3.746.87-4.944 1.15l-.8.185c-.3.07-.436-.061-.224-.285.282-.297.463-.627.525-1.173.067-.575-.128-1.285-.24-1.561-.208-.512-.558-.916-.963-1.055-.632-.217-1.081.178-.857.867.335 1.03.115 1.875-.463 2.385-1.36 1.199-1.993 2.053-1.572 3.874.46 1.986 2.432 3.265 4.418 2.805l1.75-.379z" fill="#F19020"/>
<path d="M7.164 5.528a.514.514 0 0 1-.217-.221.444.444 0 0 1 .219-.618c2.37-1.017 4.277-.024 4.357.019.24.128.338.415.217.64-.12.226-.411.304-.652.178-.068-.035-1.573-.796-3.487.026a.508.508 0 0 1-.437-.024zm6.742 2.084a.513.513 0 0 1-.201-.236.445.445 0 0 1 .26-.602c2.434-.853 4.27.267 4.346.315.23.144.308.437.173.654-.134.217-.43.275-.661.133-.067-.04-1.515-.901-3.482-.211a.514.514 0 0 1-.435-.053z" fill="#65471B"/>
</svg>

Before

Width:  |  Height:  |  Size: 1.7 KiB

After

Width:  |  Height:  |  Size: 1.6 KiB

View file

@ -41,7 +41,7 @@ module Api
def show
@article = Article.published
.includes(:user)
.includes(user: :profile)
.select(SHOW_ATTRIBUTES_FOR_SERIALIZATION)
.find(params[:id])
.decorate

View file

@ -12,7 +12,7 @@ module Api
commentable = params[:a_id] ? Article.find(params[:a_id]) : PodcastEpisode.find(params[:p_id])
@comments = commentable.comments
.includes(:user)
.includes(user: :profile)
.select(ATTRIBUTES_FOR_SERIALIZATION)
.arrange
@ -21,7 +21,7 @@ module Api
def show
tree_with_root_comment = Comment.subtree_of(params[:id].to_i(26))
.includes(:user)
.includes(user: :profile)
.select(ATTRIBUTES_FOR_SERIALIZATION)
.arrange

View file

@ -25,7 +25,7 @@ module Api
def index
@listings = Listing.published
.select(ATTRIBUTES_FOR_SERIALIZATION)
.includes(:user, :organization, :taggings, :listing_category)
.includes([{ user: :profile }, :organization, :taggings, :listing_category])
if params[:category].present?
@listings = @listings.in_category(params[:category])

View file

@ -58,7 +58,7 @@ class CommentsController < ApplicationController
def create
rate_limit!(rate_limit_to_use)
@comment = Comment.new(permitted_attributes(Comment))
@comment = Comment.includes(user: :profile).new(permitted_attributes(Comment))
@comment.user_id = current_user.id
authorize @comment

View file

@ -33,7 +33,8 @@ class StoriesController < ApplicationController
def show
@story_show = true
if (@article = Article.find_by(path: "/#{params[:username].downcase}/#{params[:slug]}")&.decorate)
path = "/#{params[:username].downcase}/#{params[:slug]}"
if (@article = Article.includes(user: :profile).find_by(path: path)&.decorate)
handle_article_show
elsif (@article = Article.find_by(slug: params[:slug])&.decorate)
handle_possible_redirect

View file

@ -300,7 +300,7 @@ class UsersController < ApplicationController
end
def default_suggested_users
@default_suggested_users ||= User.where(username: @suggested_users)
@default_suggested_users ||= User.includes(:profile).where(username: @suggested_users)
end
def determine_follow_suggestions(current_user)

View file

@ -5,38 +5,6 @@ class User < ApplicationRecord
include CloudinaryHelper
include Storext.model
# @citizen428 Preparing to drop profile columns from the users table
PROFILE_COLUMNS = %w[
available_for
behance_url
bg_color_hex
currently_hacking_on
currently_learning
dribbble_url
education
email_public
employer_name
employer_url
employment_title
facebook_url
gitlab_url
instagram_url
linkedin_url
location
mastodon_url
medium_url
mostly_work_with
stackoverflow_url
summary
text_color_hex
twitch_url
twitch_username
website_url
youtube_url
].freeze
self.ignored_columns = PROFILE_COLUMNS
# NOTE: we are using an inline module to keep profile related things together.
concerning :Profiles do
included do

View file

@ -46,7 +46,7 @@ class ArticleApiIndexService
DEFAULT_PER_PAGE
end
if (user = User.find_by(username: username))
if (user = User.includes(:profile).find_by(username: username))
user.articles.published
.includes(:organization)
.order(published_at: :desc)
@ -54,7 +54,7 @@ class ArticleApiIndexService
.per(per_page || num)
elsif (organization = Organization.find_by(slug: username))
organization.articles.published
.includes(:user)
.includes(user: :profile)
.order(published_at: :desc)
.page(page)
.per(per_page || num)
@ -64,7 +64,7 @@ class ArticleApiIndexService
end
def tag_articles
articles = Article.published.cached_tagged_with(tag).includes(:user, :organization)
articles = published_articles_with_users_and_organizations.cached_tagged_with(tag)
articles = if Tag.find_by(name: tag)&.requires_approval
articles.where(approved: true).order(featured_number: :desc)
@ -79,7 +79,7 @@ class ArticleApiIndexService
end
def tagged_articles
articles = Article.published.includes(:user, :organization)
articles = published_articles_with_users_and_organizations
articles = articles.tagged_with(tags, any: true).unscope(:select) if tags
articles = articles.tagged_with(tags_exclude, exclude: true) if tags_exclude
@ -89,14 +89,14 @@ class ArticleApiIndexService
end
def top_articles
Article.published.includes(:user, :organization)
published_articles_with_users_and_organizations
.where("published_at > ?", top.to_i.days.ago)
.order(public_reactions_count: :desc)
.page(page).per(per_page || DEFAULT_PER_PAGE)
end
def state_articles(state)
articles = Article.published.includes(:user, :organization)
articles = published_articles_with_users_and_organizations
articles = case state
when "fresh"
@ -118,9 +118,8 @@ class ArticleApiIndexService
end
def collection_articles(collection_id)
Article.published
published_articles_with_users_and_organizations
.where(collection_id: collection_id)
.includes(:user, :organization)
.order(:published_at)
.page(page)
.per(per_page || DEFAULT_PER_PAGE)
@ -129,8 +128,7 @@ class ArticleApiIndexService
def sorted_articles(sort)
# This could be expanded to allow additional sorting options
if sort == "desc"
Article.published
.includes(:user, :organization)
published_articles_with_users_and_organizations
.order(published_at: :desc)
.page(page)
.per(per_page || DEFAULT_PER_PAGE)
@ -140,11 +138,14 @@ class ArticleApiIndexService
end
def base_articles
Article.published
published_articles_with_users_and_organizations
.where(featured: true)
.includes(:user, :organization)
.order(hotness_score: :desc)
.page(page)
.per(per_page || DEFAULT_PER_PAGE)
end
def published_articles_with_users_and_organizations
Article.published.includes([{ user: :profile }, :organization])
end
end

View file

@ -34,13 +34,13 @@ module Users
def recent_producers(num_weeks = 1)
relation_as_array(
User.where(id: tagged_article_user_ids(num_weeks)),
user_relation.where(id: tagged_article_user_ids(num_weeks)),
limit: 80,
)
end
def recent_top_producers
relation = User.where(
relation = user_relation.where(
articles_count: established_user_article_count..,
comments_count: established_user_comment_count..,
)
@ -48,7 +48,7 @@ module Users
end
def recent_commenters(num_comments = 2, limit = 8)
relation_as_array(User.where(comments_count: num_comments + 1..), limit: limit)
relation_as_array(user_relation.where(comments_count: num_comments + 1..), limit: limit)
end
def relation_as_array(relation, limit:)
@ -58,13 +58,13 @@ module Users
def established_user_article_count
Rails.cache.fetch("established_user_article_count", expires_in: 1.day) do
User.where(articles_count: 1..).average(:articles_count) || User.average(:articles_count)
user_relation.where(articles_count: 1..).average(:articles_count) || User.average(:articles_count)
end
end
def established_user_comment_count
Rails.cache.fetch("established_user_comment_count", expires_in: 1.day) do
User.where(comments_count: 1..).average(:comments_count) || User.average(:comments_count)
user_relation.where(comments_count: 1..).average(:comments_count) || User.average(:comments_count)
end
end
@ -73,5 +73,9 @@ module Users
Article.where(score: 0..).average(:score) || Article.average(:score)
end
end
def user_relation
User.includes(:profile)
end
end
end

View file

@ -80,12 +80,11 @@ Rails.application.configure do
# Supress incorrect warnings from Bullet due to included columns: https://github.com/flyerhzm/bullet/issues/147
Bullet.add_safelist(type: :unused_eager_loading, class_name: "Article", association: :top_comments)
Bullet.add_safelist(type: :unused_eager_loading, class_name: "Comment", association: :user)
# NOTE: @citizen428 Temporarily ignoring this while working out user - profile relationship
Bullet.add_whitelist(type: :n_plus_one_query, class_name: "User", association: :profile)
Bullet.add_whitelist(type: :n_plus_one_query, class_name: "User", association: :setting)
Bullet.add_whitelist(type: :n_plus_one_query, class_name: "User", association: :notification_setting)
# NOTE: @citizen428 Let's ignore this for now, we have to revisit the user - profile relationship anyway
Bullet.add_whitelist(type: :n_plus_one_query, class_name: "Profile", association: :user)
# @citizen428: We have not yet resolved all user - profile preloads related to profilge generalization
Bullet.add_safelist(type: :n_plus_one_query, class_name: "User", association: :profile)
Bullet.add_safelist(type: :n_plus_one_query, class_name: "Profile", association: :user)
Bullet.add_safelist(type: :n_plus_one_query, class_name: "User", association: :setting)
Bullet.add_safelist(type: :n_plus_one_query, class_name: "User", association: :notification_setting)
# @mstruve: These occur during setting updates, not sure how since we are only dealing with single setting records
Bullet.add_safelist(type: :n_plus_one_query, class_name: "Users::Setting", association: :user)
Bullet.add_safelist(type: :n_plus_one_query, class_name: "Users::NotificationSetting", association: :user)

View file

@ -52,15 +52,6 @@ end
##############################################################################
# NOTE: @citizen428 For the time being we want all current DEV profile fields.
# The CSV import is idempotent by itself, since it uses find_or_create_by.
seeder.create("Creating DEV profile fields") do
dev_fields_csv = Rails.root.join("lib/data/dev_profile_fields.csv")
ProfileFields::ImportFromCsv.call(dev_fields_csv)
end
##############################################################################
num_users = 10 * SEEDS_MULTIPLIER
users_in_random_order = seeder.create_if_none(User, num_users) do

View file

@ -1,6 +0,0 @@
Education,text_field,,,Work,header,false
Skills/Languages,text_area,,What tools and languages are you most experienced with? Are you specialized or more of a generalist?,Coding,left_sidebar,false
Currently learning,text_area,,"What are you learning right now? What are the new tools and languages you're picking up right now?",Coding,left_sidebar,false
Currently hacking on,text_area,,What projects are currently occupying most of your time?,Coding,left_sidebar,false
Available for,text_area,,"What kinds of collaborations or discussions are you available for? What's a good reason to say Hey! to you these days?",Coding,left_sidebar,false
Work,text_field,"What do you do? Example: CEO at ACME Inc.",,Work,header,false
1 Education text_field Work header false
2 Skills/Languages text_area What tools and languages are you most experienced with? Are you specialized or more of a generalist? Coding left_sidebar false
3 Currently learning text_area What are you learning right now? What are the new tools and languages you're picking up right now? Coding left_sidebar false
4 Currently hacking on text_area What projects are currently occupying most of your time? Coding left_sidebar false
5 Available for text_area What kinds of collaborations or discussions are you available for? What's a good reason to say Hey! to you these days? Coding left_sidebar false
6 Work text_field What do you do? Example: CEO at ACME Inc. Work header false

View file

@ -1,10 +1,7 @@
module DataUpdateScripts
class CreateProfileFields
def run
# NOTE: the CSV importer uses find_or_create_by for both fields and
# groups, so this operation is idempotent.
csv = Rails.root.join("lib/data/dev_profile_fields.csv")
ProfileFields::ImportFromCsv.call(csv)
# This is no longer needed
end
end
end

View file

@ -1,35 +0,0 @@
require "rails_helper"
require Rails.root.join("lib/data_update_scripts/20200901040521_create_profile_fields.rb")
describe DataUpdateScripts::CreateProfileFields do
def profile_field_and_group_count
[ProfileField.count, ProfileFieldGroup.count]
end
before do
ProfileFieldGroup.destroy_all
ProfileField.destroy_all
end
context "when no profile fields or groups exist" do
it "creates all profile fields and groups" do
expect do
described_class.new.run
end.to change { profile_field_and_group_count }.from([0, 0]).to([6, 2])
end
end
context "when profile fields and/or groups already exist" do
before do
csv = Rails.root.join("lib/data/dev_profile_fields.csv")
ProfileFields::ImportFromCsv.call(csv)
end
it "works when profile fields or groups already exist", :aggregate_failures do
expect do
described_class.new.run
end.not_to change { profile_field_and_group_count }
expect(profile_field_and_group_count).to eq [6, 2]
end
end
end

View file

@ -11,7 +11,13 @@ RSpec.describe ProfileField, type: :model do
it { is_expected.to validate_presence_of(:display_area) }
it { is_expected.to validate_presence_of(:input_type) }
it { is_expected.to validate_presence_of(:label) }
it { is_expected.to validate_uniqueness_of(:label).case_insensitive }
end
it "ensures the label is case-insensitively unique" do
create(:profile_field, label: "Test")
expect do
described_class.create!(label: "tEsT")
end.to raise_error(ActiveRecord::RecordInvalid, /Label has already been taken/)
end
describe "#maximum_header_field_count" do
@ -29,7 +35,7 @@ RSpec.describe ProfileField, type: :model do
it "limits the number of header fields on update", :aggregate_errors do
expect(described_class.header.count).to be >= 3
profile_field = described_class.left_sidebar.first
profile_field = create(:profile_field, display_area: :left_sidebar)
expect { profile_field.header! }
.to raise_error(ActiveRecord::RecordInvalid, expected_message)

View file

@ -32,16 +32,20 @@ RSpec.describe Profile, type: :model do
end
describe "validating text areas" do
before do
create(:profile_field, label: "Test Text Area", input_type: :text_area)
end
it "is valid if the text is short enough" do
profile.skills_languages = "Ruby"
profile.test_text_area = "Ruby"
expect(profile).to be_valid
end
it "is invalid if the text is too long" do
profile.skills_languages = "x" * ProfileValidator::MAX_TEXT_AREA_LENGTH.next
profile.test_text_area = "x" * ProfileValidator::MAX_TEXT_AREA_LENGTH.next
expect(profile).not_to be_valid
expect(profile.errors_as_sentence)
.to eq "Skills languages is too long (maximum is 200 characters)"
.to eq "Test text area is too long (maximum is 200 characters)"
end
end

View file

@ -825,7 +825,7 @@ RSpec.describe User, type: :model do
it "automatically creates a profile for new users", :aggregate_failures do
user = create(:user)
expect(user.profile).to be_present
expect(user.profile).to respond_to(:available_for)
expect(user.profile).to respond_to(:location)
end
end
end

View file

@ -103,12 +103,6 @@ RSpec.configure do |config|
# Set the TZ ENV variable with the current random timezone from zonebie
# which we can then use to properly set the browser time for Capybara specs
ENV["TZ"] = Time.zone.tzinfo.name
# NOTE: @citizen428 needed while we delegate from User to Profile to keep
# spec changes limited for the time being.
csv = Rails.root.join("lib/data/dev_profile_fields.csv")
ProfileFields::ImportFromCsv.call(csv)
Profile.refresh_attributes!
end
config.before do

View file

@ -44,6 +44,11 @@ RSpec.describe "ProfilePreviewCards", type: :request do
end
describe "GET /:id as JSON" do
before do
create(:profile_field, label: "Education", display_area: :header)
create(:profile_field, label: "Work", display_area: :header)
end
let(:profile) { user.profile }
context "when signed out" do

View file

@ -187,10 +187,12 @@ RSpec.describe "StoriesShow", type: :request do
end
it "handles invalid slug characters" do
allow(Article).to receive(:find_by).and_raise(ArgumentError)
# rubocop:disable RSpec/MessageChain
allow(Article).to receive_message_chain(:includes, :find_by).and_raise(ArgumentError)
# rubocop:enable RSpec/MessageChain
get article.path
expect(response.status).to be(400)
expect(response.status).to eq(400)
end
it "has noindex if article has low score" do

View file

@ -38,10 +38,10 @@ RSpec.describe "UsersOnboarding", type: :request do
end
it "updates the user's profile" do
params = { profile: { work: "Emperor at Galactic Empire" } }
params = { profile: { location: "Galactic Empire" } }
expect do
patch "/onboarding_update.json", params: params
end.to change(user.profile, :work).to("Emperor at Galactic Empire")
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

View file

@ -11,7 +11,7 @@ RSpec.describe Users::Update, type: :service do
end
let(:profile) do
create(:profile, data: { education: "maybe", removed: "Bla" })
create(:profile, data: { test_field: "maybe", removed: "Bla" })
end
let(:user) { profile.user }
@ -33,8 +33,9 @@ RSpec.describe Users::Update, type: :service do
end
it "updates the profile_updated_at column" do
create(:profile_field, label: "Test field")
expect do
described_class.call(user, profile: { education: "false" })
described_class.call(user, profile: { test_field: "false" })
end.to change { user.reload.profile_updated_at }
end

View file

@ -17,12 +17,10 @@ Settings::SMTP.password = "password"
##############################################################################
# NOTE: @citizen428 For the time being we want all current DEV profile fields.
# The CSV import is idempotent by itself, since it uses find_or_create_by.
seeder.create("Creating DEV profile fields") do
dev_fields_csv = Rails.root.join("lib/data/dev_profile_fields.csv")
ProfileFields::ImportFromCsv.call(dev_fields_csv)
end
# Some of our Cypress tests assume specific DEV profile fields to exist
ProfileField.create!(label: "Work", display_area: :header)
ProfileField.create!(label: "Education", display_area: :header)
Profile.refresh_attributes!
##############################################################################

View file

@ -9,7 +9,8 @@ RSpec.describe Moderator::BanishUserWorker, type: :worker do
let(:admin) { create(:user, :super_admin) }
before do
user.profile.update!(currently_hacking_on: "text is here")
create(:profile_field, label: "Test field")
user.profile.update!(test_field: "text is here")
create(:article, user_id: user.id)
create(:article, user_id: user.id)
create(:listing, user: user)
@ -34,7 +35,7 @@ RSpec.describe Moderator::BanishUserWorker, type: :worker do
end
it "reassigns profile info" do
expect(user.profile.currently_hacking_on).to be_blank
expect(user.profile.test_field).to be_blank
end
it "creates an entry in the BanishedUsers table" do