From 43ccdb31f19816e9e99fe06b1d343091758eca5d Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Fri, 30 Jul 2021 10:28:40 +0000 Subject: [PATCH] Remove duplicated work display from header / profile work (#14210) * 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 outdated guard clause * Re-add validation * Update header field validation * Fix auto-complete fail --- app/controllers/stories_controller.rb | 18 +----- app/models/organization.rb | 6 ++ app/models/profile.rb | 8 +-- app/models/profile_field.rb | 20 +++++++ app/models/user.rb | 8 ++- app/serializers/search/user_serializer.rb | 3 - .../profile_preview_cards/show.json.jbuilder | 4 +- .../shared/_profile_card_content.html.erb | 59 +++++++------------ app/views/users/_meta.html.erb | 6 +- app/views/users/_metadata.html.erb | 19 +----- app/views/users/show.html.erb | 6 +- .../articleFlows/previewProfile.spec.js | 6 +- db/seeds.rb | 14 +++-- lib/data/dev_profile_fields.csv | 4 +- ...210630041322_migrate_data_to_work_field.rb | 19 ------ ...0712044513_work_profile_field_follow_up.rb | 17 ++++++ spec/factories/profile_fields.rb | 5 ++ spec/fixtures/files/profile_fields.csv | 4 +- .../add_work_profile_field_spec.rb | 2 + .../create_profile_fields_spec.rb | 4 +- .../migrate_data_to_work_field_spec.rb | 43 -------------- .../work_profile_field_follow_up_spec.rb | 23 ++++++++ spec/models/profile_field_spec.rb | 28 +++++++++ spec/requests/profile_preview_cards_spec.rb | 8 +-- spec/requests/user/user_show_spec.rb | 8 --- spec/requests/users_onboarding_spec.rb | 4 +- .../profile_fields/import_from_csv_spec.rb | 2 +- spec/support/seeds/seeds_e2e.rb | 14 +++-- spec/system/user/user_edits_profile_spec.rb | 9 --- .../moderator/banish_user_worker_spec.rb | 5 +- 30 files changed, 176 insertions(+), 200 deletions(-) delete mode 100644 lib/data_update_scripts/20210630041322_migrate_data_to_work_field.rb create mode 100644 lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb delete mode 100644 spec/lib/data_update_scripts/migrate_data_to_work_field_spec.rb create mode 100644 spec/lib/data_update_scripts/work_profile_field_follow_up_spec.rb diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 0fd7b25f7..58022f89d 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -353,9 +353,7 @@ class StoriesController < ApplicationController image: Images::Profile.call(@user.profile_image_url, length: 320), name: @user.name, email: @user.setting.display_email_on_profile ? @user.email : nil, - jobTitle: @user.employment_title.presence, - description: @user.summary.presence || "404 bio not found", - worksFor: [user_works_for].compact, + description: @user.profile.summary.presence || "404 bio not found", alumniOf: @user.education.presence }.reject { |_, v| v.blank? } end @@ -421,25 +419,13 @@ class StoriesController < ApplicationController } end - def user_works_for - # For further examples of the worksFor properties, please refer to this - # link: https://jsonld.com/person/ - return unless @user.employer_name.presence || @user.employer_url.presence - - { - "@type": "Organization", - name: @user.employer_name, - url: @user.employer_url - }.reject { |_, v| v.blank? } - end - def user_same_as # For further information on the sameAs property, please refer to this link: # https://schema.org/sameAs [ @user.twitter_username.present? ? "https://twitter.com/#{@user.twitter_username}" : nil, @user.github_username.present? ? "https://github.com/#{@user.github_username}" : nil, - @user.website_url, + @user.profile.website_url, ].reject(&:blank?) end diff --git a/app/models/organization.rb b/app/models/organization.rb index 2164152cd..83051975a 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -120,6 +120,12 @@ class Organization < ApplicationRecord organization_memberships.count == 1 && articles.count.zero? && credits.count.zero? end + # NOTE: We use Organization and User objects interchangeably. Since the former + # don't have profiles we return self instead. + def profile + self + end + private def evaluate_markdown diff --git a/app/models/profile.rb b/app/models/profile.rb index c42a1cfb9..6eede755b 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -11,13 +11,7 @@ class Profile < ApplicationRecord # any profile on a given Forem. STATIC_FIELDS = %w[summary location website_url].freeze - SPECIAL_DISPLAY_ATTRIBUTES = %w[ - summary - employment_title - employer_name - employer_url - location - ].freeze + SPECIAL_DISPLAY_ATTRIBUTES = %w[summary location].freeze # NOTE: @citizen428 This is a temporary mapping so we don't break DEV during # profile migration/generalization work. diff --git a/app/models/profile_field.rb b/app/models/profile_field.rb index c9d67ba84..26390f289 100644 --- a/app/models/profile_field.rb +++ b/app/models/profile_field.rb @@ -1,6 +1,9 @@ class ProfileField < ApplicationRecord WORD_REGEX = /\b\w+\b/.freeze + HEADER_FIELD_LIMIT = 3 + HEADER_LIMIT_MESSAGE = "maximum number of header fields (#{HEADER_FIELD_LIMIT}) exceeded".freeze + # Key names follow the Rails form helpers enum input_type: { text_field: 0, @@ -21,6 +24,7 @@ class ProfileField < ApplicationRecord validates :input_type, presence: true validates :label, presence: true, uniqueness: { case_sensitive: false } validates :show_in_onboarding, inclusion: { in: [true, false] } + validate :maximum_header_field_count before_create :generate_attribute_name @@ -35,4 +39,20 @@ class ProfileField < ApplicationRecord def generate_attribute_name self.attribute_name = label.titleize.scan(WORD_REGEX).join.underscore end + + def maximum_header_field_count + return unless header? + + header_field_count = self.class.header.count + + # We need to have less than the maximum number so we can still create one. + if new_record? || display_area_was == "left_sidebar" + return if header_field_count < HEADER_FIELD_LIMIT + # We can change existing fields or update them as long as we're within the limit. + elsif header_field_count <= HEADER_FIELD_LIMIT + return + end + + errors.add(:display_area, HEADER_LIMIT_MESSAGE) + end end diff --git a/app/models/user.rb b/app/models/user.rb index 78e493f11..1a0760441 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -351,7 +351,7 @@ class User < ApplicationRecord end def processed_website_url - website_url.to_s.strip if website_url.present? + profile.website_url.to_s.strip if profile.website_url.present? end def remember_me @@ -636,8 +636,12 @@ class User < ApplicationRecord Users::BustCacheWorker.perform_async(id) end + # TODO: @citizen428 I don't want to completely remove this method yet, as we + # have similar methods in other models. But the previous implementation used + # three profile fields that we can't guarantee to exist across all Forems. So + # for now this method will just return an empty string. def tag_keywords_for_search - "#{employer_name}#{mostly_work_with}#{available_for}" + "" end # TODO: this can be removed once we migrate away from ES diff --git a/app/serializers/search/user_serializer.rb b/app/serializers/search/user_serializer.rb index 27bc9f22c..8b08a1030 100644 --- a/app/serializers/search/user_serializer.rb +++ b/app/serializers/search/user_serializer.rb @@ -3,13 +3,10 @@ module Search HASH_TRANSFORM = ->(key, value) { { name: key, value: value } } attributes :id, - :available_for, :comments_count, :badge_achievements_count, - :employer_name, :hotness_score, :last_comment_at, - :mostly_work_with, :name, :path, :public_reactions_count, diff --git a/app/views/profile_preview_cards/show.json.jbuilder b/app/views/profile_preview_cards/show.json.jbuilder index f3b1ac9f4..65ae4763f 100644 --- a/app/views/profile_preview_cards/show.json.jbuilder +++ b/app/views/profile_preview_cards/show.json.jbuilder @@ -1,6 +1,8 @@ +# TODO: @citizen428 - We shouldn't use education and work directly here, since +# we can't guarantee that these profile fields will exist on all Forems. json.extract!( @user.profile, - :summary, :employment_title, :employer_name, :employer_url, :location, :education + :summary, :location, :education, :work ) json.card_color( diff --git a/app/views/shared/_profile_card_content.html.erb b/app/views/shared/_profile_card_content.html.erb index 191103739..512d7e640 100644 --- a/app/views/shared/_profile_card_content.html.erb +++ b/app/views/shared/_profile_card_content.html.erb @@ -29,24 +29,6 @@ <% end %> - <% if actor.employment_title.present? %> -
  • -
    - Work -
    -
    - <%= actor.employment_title %> - <% if actor.employer_name.present? %> - at - <% if actor.employer_url.present? %> - <%= actor.employer_name %> - <% else %> - <%= actor.employer_name %> - <% end %> - <% end %> -
    -
    • - <% end %> <% if actor.location.present? %>
  • @@ -57,26 +39,29 @@
    • <% end %> - <% if actor.education.present? %> -
  • -
    - Education -
    -
    - <%= actor.education %> -
    -
    • + <% if (header_fields = actor.profile.decorate.ui_attributes_for(area: :header)).present? %> + <% header_fields.sort.each do |title, value| %> + <% next if Profile.special_attributes.include?(title) %> +
  • +
    + <%= title %> +
    +
    + <%= value %> +
    +
    • + <% end %> <% end %> -
  • -
    - Joined -
    -
    - <%= local_date(actor.created_at) %> -
    -
    • - - +
  • +
    + Joined +
    +
    + <%= local_date(actor.created_at) %> +
    +
    • + + <% elsif actor.class.name == "Organization" && actor.approved_and_filled_out_cta? %>
    <%= sanitize_rendered_markdown(actor.cta_processed_html) %> diff --git a/app/views/users/_meta.html.erb b/app/views/users/_meta.html.erb index ecdb2a753..d99529d31 100644 --- a/app/views/users/_meta.html.erb +++ b/app/views/users/_meta.html.erb @@ -1,19 +1,19 @@ - + <%= meta_keywords_default %> - + "> - + <% if @stories.any? %> <%= auto_discovery_link_tag(:rss, app_url("/feed/#{@user.username}"), title: "#{community_name} RSS Feed") %> diff --git a/app/views/users/_metadata.html.erb b/app/views/users/_metadata.html.erb index 4bcac82dd..fd615a18d 100644 --- a/app/views/users/_metadata.html.erb +++ b/app/views/users/_metadata.html.erb @@ -1,8 +1,7 @@ <% if (header_fields = @profile.ui_attributes_for(area: :header)).present? %>
    - <% header_fields.each do |title, value| %> + <% header_fields.sort.each do |title, value| %> <% next if Profile.special_attributes.include?(title) %> - <% next if title == "work" %>
    <%= sanitized_sidebar title.titleize %> @@ -12,21 +11,5 @@

    <% end %> - <% if @user.employment_title.present? %> -
    - Work -

    - <%= @user.employment_title %> - <% if @user.employer_name.present? %> - at - <% if @user.employer_url.present? %> - <%= @user.employer_name %> - <% else %> - <%= @user.employer_name %> - <% end %> - <% end %> -

    -
    - <% end %>
    <% end %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 86c3b2e30..5c9f20538 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -60,10 +60,10 @@

    <%= @user.name %>

    -

    <%= @user.summary.presence || ["404 bio not found"].sample %>

    +

    <%= @user.profile.summary.presence || ["404 bio not found"].sample %>

    - <% if @user.location.present? %> + <% if @user.profile.location.present? %> <%= inline_svg_tag("location.svg", aria: true, class: "crayons-icon mr-2 shrink-0", title: "Location") %> <%= @user.location %> @@ -83,7 +83,7 @@ <% end %> - <% if @user.website_url.present? %> + <% if @user.profile.website_url.present? %> <%= inline_svg_tag("link-external.svg", class: "crayons-icon mr-2 shrink-0", aria: true, title: "Personal website") %> <%= @user.website_url %> diff --git a/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js b/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js index 2d3a4e703..dbb0c339a 100644 --- a/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js +++ b/cypress/integration/seededFlows/articleFlows/previewProfile.spec.js @@ -77,7 +77,7 @@ describe('Preview user profile from article page', () => { // Check all the expected user data sections are present cy.findByText('Admin user summary'); - cy.findByText('Software developer'); + cy.findByText('Software developer at Company'); cy.findByText('Edinburgh'); cy.findByText('University of Life'); @@ -116,7 +116,7 @@ describe('Preview user profile from article page', () => { // Check all the expected user data sections are present cy.findByText('Admin user summary'); - cy.findByText('Software developer'); + cy.findByText('Software developer at Company'); cy.findByText('Edinburgh'); cy.findByText('University of Life'); @@ -281,7 +281,7 @@ describe('Preview user profile from article page', () => { cy.findByRole('button', { name: 'Follow' }); cy.findByText('Admin user summary'); - cy.findByText('Software developer'); + cy.findByText('Software developer at Company'); cy.findByText('Edinburgh'); cy.findByText('University of Life'); }); diff --git a/db/seeds.rb b/db/seeds.rb index 5f207bac7..53ceec498 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -71,9 +71,7 @@ users_in_random_order = seeder.create_if_none(User, num_users) do user = User.create!( name: name, - summary: Faker::Lorem.paragraph_by_chars(number: 199, supplemental: false), profile_image: File.open(Rails.root.join("app/assets/images/#{rand(1..40)}.png")), - website_url: Faker::Internet.url, twitter_username: Faker::Internet.username(specifier: name), # Emails limited to 50 characters email: Faker::Internet.email(name: name, separators: "+", domain: Faker::Internet.domain_word.first(20)), @@ -84,6 +82,11 @@ users_in_random_order = seeder.create_if_none(User, num_users) do password_confirmation: "password", ) + user.profile.update( + summary: Faker::Lorem.paragraph_by_chars(number: 199, supplemental: false), + website_url: Faker::Internet.url, + ) + if i.zero? user.add_role(:trusted) # guarantee at least one moderator elsif i == num_users - 1 @@ -137,14 +140,17 @@ seeder.create_if_doesnt_exist(User, "email", "admin@forem.local") do name: "Admin McAdmin", email: "admin@forem.local", username: "Admin_McAdmin", - summary: Faker::Lorem.paragraph_by_chars(number: 199, supplemental: false), profile_image: File.open(Rails.root.join("app/assets/images/#{rand(1..40)}.png")), - website_url: Faker::Internet.url, confirmed_at: Time.current, password: "password", password_confirmation: "password", ) + user.profile.update( + summary: Faker::Lorem.paragraph_by_chars(number: 199, supplemental: false), + website_url: Faker::Internet.url, + ) + user.add_role(:super_admin) user.add_role(:tech_admin) end diff --git a/lib/data/dev_profile_fields.csv b/lib/data/dev_profile_fields.csv index 5bad9ec51..c6a85679d 100644 --- a/lib/data/dev_profile_fields.csv +++ b/lib/data/dev_profile_fields.csv @@ -1,8 +1,6 @@ Education,text_field,,,Work,header,false -Employer name,text_field,Acme Inc.,,Work,header,false -Employer URL,text_field,https://dev.com,,Work,header,false -Employment title,text_field,Junior Frontend Engineer,,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 diff --git a/lib/data_update_scripts/20210630041322_migrate_data_to_work_field.rb b/lib/data_update_scripts/20210630041322_migrate_data_to_work_field.rb deleted file mode 100644 index d24b91d74..000000000 --- a/lib/data_update_scripts/20210630041322_migrate_data_to_work_field.rb +++ /dev/null @@ -1,19 +0,0 @@ -module DataUpdateScripts - class MigrateDataToWorkField - # NOTE: data->>'employment_title' <> '' takes care of both null and empty strings. - # This works for two reasons: - # 1. We use the JSONB ->> operator, which coerces the result to text. This also - # turns JSONB `null` into "Postgres" `null`. - # See: https://www.postgresql.org/docs/13/functions-json.html - # 2. `null` is neither equal to nor unequal to any string. For this reason - # `stringexpression <> ''` can be used to filter both `null`s and empty strings. - # See: https://stackoverflow.com/questions/23766084/best-way-to-check-for-empty-or-null-value - QUERY = "data->>'employment_title' <> '' AND data->>'work' IS NULL".freeze - - def run - Profile.where(QUERY).select(:id).find_each do |profile| - MigrateDataToWorkFieldWorker.perform_async(profile.id) - end - end - end -end diff --git a/lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb b/lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb new file mode 100644 index 000000000..a0cbb550f --- /dev/null +++ b/lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb @@ -0,0 +1,17 @@ +module DataUpdateScripts + class WorkProfileFieldFollowUp + OBSOLETE_FIELDS = %w[employer_name employer_url employment_title].freeze + + def run + ProfileField.destroy_by(attribute_name: OBSOLETE_FIELDS) + + work_field = ProfileField.find_by(attribute_name: "work") + return unless work_field + + work_group = ProfileFieldGroup.find_by(name: "Work") + return unless work_group + + work_field.update(profile_field_group_id: work_group.id) + end + end +end diff --git a/spec/factories/profile_fields.rb b/spec/factories/profile_fields.rb index 2da6faee9..892c3fd8c 100644 --- a/spec/factories/profile_fields.rb +++ b/spec/factories/profile_fields.rb @@ -6,11 +6,16 @@ FactoryBot.define do description { "some description" } placeholder_text { "john.doe@example.com" } show_in_onboarding { false } + display_area { :left_sidebar } trait :onboarding do show_in_onboarding { true } end + trait :header do + display_area { :header } + end + after :create do # this is accomplished by ProfileFields::Add normally, it was added here # in case the tests use the factory and not the service object diff --git a/spec/fixtures/files/profile_fields.csv b/spec/fixtures/files/profile_fields.csv index 3635dc54e..41de87861 100644 --- a/spec/fixtures/files/profile_fields.csv +++ b/spec/fixtures/files/profile_fields.csv @@ -1,4 +1,4 @@ -Full test,text_field,Test,Test field,Basic,header,true -Test name,text_field,John Doe,,Basic,header,false +Full test,text_field,Test,Test field,Basic,left_sidebar,true +Test name,text_field,John Doe,,Basic,left_sidebar,false Test languages,text_area,,"Programming languages, frameworks, etc.",Coding,left_sidebar,false diff --git a/spec/lib/data_update_scripts/add_work_profile_field_spec.rb b/spec/lib/data_update_scripts/add_work_profile_field_spec.rb index 5fe21f6b4..7c2a2daab 100644 --- a/spec/lib/data_update_scripts/add_work_profile_field_spec.rb +++ b/spec/lib/data_update_scripts/add_work_profile_field_spec.rb @@ -4,6 +4,8 @@ require Rails.root.join( ) describe DataUpdateScripts::AddWorkProfileField do + before { ProfileField.destroy_by(label: "Work") } + it "adds a new profile field" do expect { described_class.new.run }.to change(ProfileField, :count).by(1) end diff --git a/spec/lib/data_update_scripts/create_profile_fields_spec.rb b/spec/lib/data_update_scripts/create_profile_fields_spec.rb index 8eeacd5f4..01a147d56 100644 --- a/spec/lib/data_update_scripts/create_profile_fields_spec.rb +++ b/spec/lib/data_update_scripts/create_profile_fields_spec.rb @@ -15,7 +15,7 @@ describe DataUpdateScripts::CreateProfileFields 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([8, 2]) + end.to change { profile_field_and_group_count }.from([0, 0]).to([6, 2]) end end @@ -29,7 +29,7 @@ describe DataUpdateScripts::CreateProfileFields do expect do described_class.new.run end.not_to change { profile_field_and_group_count } - expect(profile_field_and_group_count).to eq [8, 2] + expect(profile_field_and_group_count).to eq [6, 2] end end end diff --git a/spec/lib/data_update_scripts/migrate_data_to_work_field_spec.rb b/spec/lib/data_update_scripts/migrate_data_to_work_field_spec.rb deleted file mode 100644 index d4098067d..000000000 --- a/spec/lib/data_update_scripts/migrate_data_to_work_field_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -require "rails_helper" -require Rails.root.join( - "lib/data_update_scripts/20210630041322_migrate_data_to_work_field.rb", -) - -describe DataUpdateScripts::MigrateDataToWorkField, sidekiq: :inline do - let!(:user) { create(:user) } - let(:profile) { user.profile } - - before do - ProfileField.find_or_create_by!(label: "Work", input_type: :text_field) - Profile.refresh_attributes! - end - - it "migrates employment titles without employer name" do - profile.update!(employment_title: "Tester") - expect { described_class.new.run } - .to change { profile.reload.work }.from(nil).to("Tester") - end - - it "migrates employment titles and employer name" do - profile.update!(employment_title: "Tester", employer_name: "ACME Inc.") - expect { described_class.new.run } - .to change { profile.reload.work }.from(nil).to("Tester at ACME Inc.") - end - - it "ignores blank employment titles" do - profile.update!(employment_title: "", employer_name: "ACME Inc.") - expect { described_class.new.run }.not_to change { profile.reload.work } - end - - it "ignores blank employer names" do - profile.update!(employment_title: "Tester", employer_name: "") - expect { described_class.new.run } - .to change { profile.reload.work }.from(nil).to("Tester") - end - - # Regression spec for https://github.com/forem/forem/issues/14188 - it "does not accidentally update employment_title" do - profile.update!(employment_title: "Tester", employer_name: "ACME Inc.") - expect { described_class.new.run }.not_to change { profile.reload.employment_title } - end -end diff --git a/spec/lib/data_update_scripts/work_profile_field_follow_up_spec.rb b/spec/lib/data_update_scripts/work_profile_field_follow_up_spec.rb new file mode 100644 index 000000000..cf79f66be --- /dev/null +++ b/spec/lib/data_update_scripts/work_profile_field_follow_up_spec.rb @@ -0,0 +1,23 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb", +) + +describe DataUpdateScripts::WorkProfileFieldFollowUp do + it "removes the three obsolete profile fields" do + ProfileField.find_or_create_by(label: "Employer name") + ProfileField.find_or_create_by(label: "Employer URL") + ProfileField.find_or_create_by(label: "Employment title") + + expect { described_class.new.run }.to change(ProfileField, :count).by(-3) + end + + it "changes the group of the work field" do + work_field = ProfileField.find_or_create_by(attribute_name: "work", label: "Work") + work_field.update(profile_field_group: nil) # ensure we start without a group + work_group = ProfileFieldGroup.find_or_create_by(name: "Work") + + expect { described_class.new.run } + .to change { work_field.reload.profile_field_group }.from(nil).to(work_group) + end +end diff --git a/spec/models/profile_field_spec.rb b/spec/models/profile_field_spec.rb index 8ea7b24a4..c8cc784a0 100644 --- a/spec/models/profile_field_spec.rb +++ b/spec/models/profile_field_spec.rb @@ -13,6 +13,34 @@ RSpec.describe ProfileField, type: :model do it { is_expected.to validate_presence_of(:label) } it { is_expected.to validate_uniqueness_of(:label).case_insensitive } end + + describe "#maximum_header_field_count" do + before do + count = [0, described_class::HEADER_FIELD_LIMIT - described_class.header.count].max + create_list(:profile_field, count, :header) + end + + let(:expected_message) { /#{Regexp.quote(ProfileField::HEADER_LIMIT_MESSAGE)}/ } + + it "limits the number of header fields on create" do + expect { create(:profile_field, :header) } + .to raise_error(ActiveRecord::RecordInvalid, expected_message) + end + + 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 + + expect { profile_field.header! } + .to raise_error(ActiveRecord::RecordInvalid, expected_message) + end + + it "considers existing header fields valid even if we reached the maximum", :aggregate_errors do + expect(described_class.header.count).to be >= 3 + + expect { described_class.header.last.validate! }.not_to raise_error + end + end end describe "callbacks" do diff --git a/spec/requests/profile_preview_cards_spec.rb b/spec/requests/profile_preview_cards_spec.rb index f450dfe8c..95362c837 100644 --- a/spec/requests/profile_preview_cards_spec.rb +++ b/spec/requests/profile_preview_cards_spec.rb @@ -62,9 +62,7 @@ RSpec.describe "ProfilePreviewCards", type: :request do preview_card = response.parsed_body expect(preview_card["summary"]).to eq(profile.summary) - expect(preview_card["employment_title"]).to eq(profile.employment_title) - expect(preview_card["employer_name"]).to eq(profile.employer_name) - expect(preview_card["employer_url"]).to eq(profile.employer_url) + expect(preview_card["work"]).to eq(profile.work) expect(preview_card["location"]).to eq(profile.location) expect(preview_card["education"]).to eq(profile.education) expect(preview_card["created_at"]).to eq(profile.created_at.utc.iso8601) @@ -116,9 +114,7 @@ RSpec.describe "ProfilePreviewCards", type: :request do preview_card = response.parsed_body expect(preview_card["summary"]).to eq(profile.summary) - expect(preview_card["employment_title"]).to eq(profile.employment_title) - expect(preview_card["employer_name"]).to eq(profile.employer_name) - expect(preview_card["employer_url"]).to eq(profile.employer_url) + expect(preview_card["work"]).to eq(profile.work) expect(preview_card["location"]).to eq(profile.location) expect(preview_card["education"]).to eq(profile.education) expect(preview_card["created_at"]).to eq(profile.created_at.utc.iso8601) diff --git a/spec/requests/user/user_show_spec.rb b/spec/requests/user/user_show_spec.rb index 9f156c7e2..cea6f353a 100644 --- a/spec/requests/user/user_show_spec.rb +++ b/spec/requests/user/user_show_spec.rb @@ -38,15 +38,7 @@ RSpec.describe "UserShow", type: :request do "image" => Images::Profile.call(user.profile_image_url, length: 320), "name" => user.name, "email" => user.email, - "jobTitle" => user.employment_title, "description" => user.summary, - "worksFor" => [ - { - "@type" => "Organization", - "name" => user.employer_name, - "url" => user.employer_url - }, - ], "alumniOf" => user.education, ) end diff --git a/spec/requests/users_onboarding_spec.rb b/spec/requests/users_onboarding_spec.rb index 41998a183..76e9fa8e1 100644 --- a/spec/requests/users_onboarding_spec.rb +++ b/spec/requests/users_onboarding_spec.rb @@ -33,10 +33,10 @@ RSpec.describe "UsersOnboarding", type: :request do end it "updates the user's profile" do - params = { profile: { employer_name: "Galatic Empire" } } + params = { profile: { work: "Emperor at Galactic Empire" } } expect do patch "/onboarding_update.json", params: params - end.to change(user.profile, :employer_name).to("Galatic Empire") + end.to change(user.profile, :work).to("Emperor at Galactic Empire") end it "does not update the user's last_onboarding_page if it is empty" do diff --git a/spec/services/profile_fields/import_from_csv_spec.rb b/spec/services/profile_fields/import_from_csv_spec.rb index cec3da024..34abaa732 100644 --- a/spec/services/profile_fields/import_from_csv_spec.rb +++ b/spec/services/profile_fields/import_from_csv_spec.rb @@ -16,7 +16,7 @@ RSpec.describe ProfileFields::ImportFromCsv do expect(field.placeholder_text).to eq "Test" expect(field.description).to eq "Test field" expect(field.profile_field_group.name).to eq "Basic" - expect(field.display_area).to eq "header" + expect(field.display_area).to eq "left_sidebar" expect(field.show_in_onboarding).to be true end diff --git a/spec/support/seeds/seeds_e2e.rb b/spec/support/seeds/seeds_e2e.rb index b7c30d8c9..f610a0cff 100644 --- a/spec/support/seeds/seeds_e2e.rb +++ b/spec/support/seeds/seeds_e2e.rb @@ -38,17 +38,19 @@ seeder.create_if_doesnt_exist(User, "email", "admin@forem.local") do checked_code_of_conduct: true, checked_terms_and_conditions: true, ) + user.notification_setting.update( email_comment_notifications: false, email_follower_notifications: false, ) - user.profile.update({ - summary: "Admin user summary", - employment_title: "Software developer", - location: "Edinburgh", - education: "University of Life" - }) + user.profile.update( + summary: "Admin user summary", + work: "Software developer at Company", + location: "Edinburgh", + education: "University of Life", + ) + user.add_role(:super_admin) user.add_role(:single_resource_admin, Config) user.add_role(:trusted) diff --git a/spec/system/user/user_edits_profile_spec.rb b/spec/system/user/user_edits_profile_spec.rb index 00d453901..33b472c1b 100644 --- a/spec/system/user/user_edits_profile_spec.rb +++ b/spec/system/user/user_edits_profile_spec.rb @@ -15,12 +15,6 @@ RSpec.describe "User edits their profile", type: :system do label: "Hate Ice Cream Flavor", display_area: "header") end - let!(:settings_only_profile_field) do - create(:profile_field, - profile_field_group: profile_field_group, - label: "Imaginary Ice Cream Flavor", - display_area: "settings_only") - end before do sign_in user @@ -55,18 +49,15 @@ RSpec.describe "User edits their profile", type: :system do it "renders profile fields" do expect(page).to have_text(left_sidebar_profile_field.attribute_name.titleize) expect(page).to have_text(header_profile_field.attribute_name.titleize) - expect(page).to have_text(settings_only_profile_field.attribute_name.titleize) end it "reflects set profile fields in the interface" do fill_in "profile[#{left_sidebar_profile_field.attribute_name}]", with: "chocolate" fill_in "profile[#{header_profile_field.attribute_name}]", with: "pistachio" - fill_in "profile[#{settings_only_profile_field.attribute_name}]", with: "cthulhu" click_button "Save" visit "/#{user.username}" - expect(page).not_to have_text(settings_only_profile_field.attribute_name.titleize) expect(page).not_to have_text("cthulhu") within(".crayons-layout__sidebar-left") do diff --git a/spec/workers/moderator/banish_user_worker_spec.rb b/spec/workers/moderator/banish_user_worker_spec.rb index dbf4cd8ad..6d10e4c71 100644 --- a/spec/workers/moderator/banish_user_worker_spec.rb +++ b/spec/workers/moderator/banish_user_worker_spec.rb @@ -4,11 +4,12 @@ RSpec.describe Moderator::BanishUserWorker, type: :worker do include_examples "#enqueues_on_correct_queue", "high_priority", 1 describe "#perform" do - let(:user) { create(:user, currently_hacking_on: "text is here") } + let(:user) { create(:user) } let(:user2) { create(:user) } let(:admin) { create(:user, :super_admin) } before do + user.profile.update!(currently_hacking_on: "text is here") create(:article, user_id: user.id) create(:article, user_id: user.id) create(:listing, user: user) @@ -33,7 +34,7 @@ RSpec.describe Moderator::BanishUserWorker, type: :worker do end it "reassigns profile info" do - expect(user.currently_hacking_on).to be_blank + expect(user.profile.currently_hacking_on).to be_blank end it "creates an entry in the BanishedUsers table" do