maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Profile attribute names should be unique and non-empty (#16396)

Browse source 
* transliterate when generating attribute names to avoid emptiness

If you enter a non-ascii (non `\w` matching) string as the profile
field label, the attribute name is the empty string.

This causes problems outlined in #16391

To avoid losing user provided data, transliterate (Sterile is the same
tool we're using for Article#title_to_slug) before matching against
the word regex.

If we persist an empty string, or persist a non-`\w` name, the
coordinating regex in Profile::ATTRIBUTE_NAME_REGEX will leave a nil
match and raise NoMethodError (the method missing is for
match[:attribute_name] when match was nil, not Profile - that's the
next commit.

* Guard against nil matches

If an attribute name doesn't match the regex, the match is nil, and
trying to access (nil)[:attribute_name] raises a NoMethodError.

If there was no match, assume profile does not respond to the
selector, and don't handle it in method missing.

* Ensure generated attribute name is valid before saving

This raises a validation error if the generated name (from the label)
would be empty.

It's not an optimal error message (since the user can't see the
internal attribute name) but it prevents persisting broken/empty data

* actually raise error when validating

validate/valid? only return true or false (and set errors on the
object). In order to reject the creation, we need to raise a
validation error, not only call validate. I think this is because the
execution of before_create hooks happens after validation (which is
why the validation was only checked on update, not create).

* Generate an attribute name completely independent of the field label

This prevents mistakenly labeling a field "Class" or "Association" or
one of the other hundred public methods an AR model like Profile
exposes. Since attribute_name will be passed to `profile.public_send`
we really shouldn't build selectors from user supplied inputs.

* Use the admin supplied label in the sidebar

The profile decorator is used in the Users#show page to populate the
sidebar fields. Don't use the attribute name (which we mangled during
creation, and now generate randomly) as the label, use the label.

* Make the label lookup null safe, and filter attributes more

* Update data update script to not expect predictable labels

This is low impact since it ran in july, but we no longer know what
attribute name a label will create.

* Fix moderator spec

"Test Field" label no longer predictably generates :test_field as an
attribute name. Ask the field what it's name is before asking profile
about it.

* Fix profile preview card request spec

Remove the assumption that profile responds to a method name based on
the label for Work and Education fields.

* Update old DUS and its test

This can probably be archived

* Update profile spec to use fields generated attribute names

We used to "know" how attributes were generated from labels. Now we don't.

* update e2e seeds for profile field change

* Don't expect attribute name to be based on the label

* expect created profile fields respond to their attribute name

* Update system test

The label, not the attribute name, is shown on the profile form (the
field has an id related to the attribute name, but the view shows the
mutable/human-readable label).

* Keep the field title lowercase when sending the json preview card

The userMetadata component expects "work" and "education" to be
attributes of the metadata, but the ui_attributes_for() method was
titlizing these (for display).

Ideally we wouldn't have "special purposed" these two field names, but
they're there.

* update profile field by attribute name, not based on label

* Update profile field removal assumptions

We don't know what the method selectors will be, we have to ask.

* remove old test

* Update translations for Education and Work

Since the ui_attributes_for(area:) now gives the label, not the
attribute, we need to match the label of the profile field.

Note to self: this exposes an issue in localizing the custom profile
fields (probably a bigger problem for large, international communities
like DEV than some others, but trying to match static translation
files against user-modifiable database records seems like a problem
we'll see again).

* Empty commit to retrigger buildkite

* Remove profile field migration update scripts

Cloned the specs from the other "remove unused scripts" script.

* Remove unused scripts

The data update script removes the entry from the table (recording
that these have run) - we also want to remove the files (preventing
them from running again).

* remove unneeded spec for removed file

* when translation for header area field not found, use the title

Only Work and Education already have keys in the yml translation file,
and there's not a great (or easy?) way to make multiple translations
on these fields right now.

Since an admin can create a new field, and assign it to the header
area, we can't assume the code has a configured translation key for
this field.

Fallback to the title (we do this in another context already) if
there's no translation.

* PR feedback: Avoid n+1 query for labels

the original implementation of "label_for_attribute" had an n+1 query
looping over each matched key.

Follow suggested improvement and pull labels and attributes at once
from the db and modify the returned hash.

* Downcase title before looking for translation key

This avoids putting "odd" capitalized keys into the yml translation
file

Revert addition of "Work" and "Education" to the users files.

* use a let binding for duplicated test data

* Update app/models/profile_field.rb

prefer SecureRandom.hex for a dashless uuid (instead of removing the dashes).

Co-authored-by: Jamie Gaskins <jgaskins@hey.com>

Co-authored-by: Jamie Gaskins <jgaskins@hey.com>
This commit is contained in:
Daniel Uber 2022-04-04 12:14:02 -05:00 • committed by GitHub
parent 1b2aa68a4d
commit 1a07ad8d9e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
25 changed files with 113 additions and 146 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/decorators/profile_decorator.rb
View file

@ -2,7 +2,7 @@ class ProfileDecorator < ApplicationDecorator
# Return a Hash of the profile fields that should be rendered for a given
# display area, e.g. :left_sidebar
def ui_attributes_for(area:)
names = ProfileField.public_send(area).pluck(:attribute_name)
data.slice(*names).select { |_, v| v.present? }
fields = ProfileField.public_send(area).pluck(:label, :attribute_name).to_h
fields.transform_values { |attribute_name| data[attribute_name] }.compact_blank
end
end

4
app/models/profile.rb
View file

@ -39,6 +39,8 @@ class Profile < ApplicationRecord
# Lazily add accessors for profile fields on first use
def method_missing(method_name, *args, **kwargs, &block)
match = method_name.match(ATTRIBUTE_NAME_REGEX)
super unless match
field = ProfileField.find_by(attribute_name: match[:attribute_name])
super unless field
@ -53,7 +55,7 @@ class Profile < ApplicationRecord
# an explicit `responds_to?` check.
def respond_to_missing?(method_name, include_private = false)
match = method_name.match(ATTRIBUTE_NAME_REGEX)
return true if match[:attribute_name].in?(self.class.attributes)
return true if match && match[:attribute_name].in?(self.class.attributes)
super
end

2
app/models/profile_field.rb
View file

@ -22,7 +22,7 @@ class ProfileField < ApplicationRecord
private
def generate_attribute_name
self.attribute_name = label.titleize.scan(WORD_REGEX).join.underscore
self.attribute_name = "attribute_#{SecureRandom.hex}"
end
def maximum_header_field_count

2
app/views/profile_preview_cards/show.json.jbuilder
View file

@ -14,5 +14,5 @@ json.created_at utc_iso_timestamp(@user.created_at)
# Dynamically add the information for the header fields (maximum of 3 fields)
header_fields = @user.profile.decorate.ui_attributes_for(area: :header)
header_fields.each do |title, value|
json.set! title, value
json.set! title.downcase, value
end

2
app/views/shared/_profile_card_content.html.erb
View file

@ -43,7 +43,7 @@
<% header_fields.sort.each do |title, value| %>
<li>
<div class="key">
<%= t("views.users.profile_fields.#{title}") %>
<%= t("views.users.profile_fields.#{title.downcase}", default: title.titleize) %>
</div>
<div class="value">
<%= value %>

16
lib/data_update_scripts/20200826075937_migrate_profile_field_groups.rb
View file

@ -1,16 +0,0 @@
module DataUpdateScripts
class MigrateProfileFieldGroups
def run
# ensure we can run this after the group column gets removed
return unless "group".in?(ProfileField.column_names)
profile_field_groups = Hash.new do |hash, key|
hash[key] = ProfileFieldGroup.find_or_create_by(name: key)
end
ProfileField.find_each do |profile_field|
profile_field.update(profile_field_group: profile_field_groups[profile_field.group])
end
end
end
end

4
lib/data_update_scripts/20201208151516_set_onboarding_profile_fields_for_existing_forems.rb
View file

@ -3,8 +3,8 @@ module DataUpdateScripts
def run
return unless User.count.positive?
ProfileField.where(attribute_name: "summary").update(label: "Bio", show_in_onboarding: true)
ProfileField.where(attribute_name: "location").update(label: "Location", show_in_onboarding: true)
ProfileField.where(label: "summary").update(label: "Bio", show_in_onboarding: true)
ProfileField.where(label: "location").update(label: "Location", show_in_onboarding: true)
ProfileField.where(attribute_name: "employment_title").update(label: "Employer title",
show_in_onboarding: true)
ProfileField.where(attribute_name: "employer_name").update(label: "Employer name", show_in_onboarding: true)

10
lib/data_update_scripts/20210108033107_remove_looking_for_work_profile_fields.rb
View file

@ -1,10 +0,0 @@
module DataUpdateScripts
class RemoveLookingForWorkProfileFields
def run
# destroy_by is idempotent by default: if no record can be found an empty
# array will be returned.
ProfileField.destroy_by(attribute_name: "looking_for_work")
ProfileField.destroy_by(attribute_name: "display_looking_for_work_on_profile")
end
end
end

12
lib/data_update_scripts/20210630034523_add_work_profile_field.rb
View file

@ -1,12 +0,0 @@
module DataUpdateScripts
class AddWorkProfileField
def run
ProfileField.find_or_create_by(
label: "Work",
placeholder_text: "What do you do? Example: CEO at ACME Inc.",
input_type: :text_field,
display_area: :header,
)
end
end
end

7
lib/data_update_scripts/20210630063635_drop_profile_fields_for_static_attributes.rb
View file

@ -1,7 +0,0 @@
module DataUpdateScripts
class DropProfileFieldsForStaticAttributes
def run
ProfileField.destroy_by(attribute_name: Profile.static_fields)
end
end
end

17
lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb
View file

@ -1,17 +0,0 @@
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

18
lib/data_update_scripts/20220214195145_remove_profile_field_update_scripts.rb Normal file
View file

@ -0,0 +1,18 @@
module DataUpdateScripts
class RemoveProfileFieldUpdateScripts
# these are safe to remove, because the seed data will have current values
# and these largely predate selfhost. The attribute name api changed substantially
# and reworking legacy scripts to match the refactor is higher cost than value
FILE_NAMES = %w[
20210108033107_remove_looking_for_work_profile_fields
20210630034523_add_work_profile_field
20210630063635_drop_profile_fields_for_static_attributes
20210712044513_work_profile_field_follow_up
20200826075937_migrate_profile_field_groups
].freeze
def run
DataUpdateScript.delete_by(file_name: FILE_NAMES)
end
end
end

16
spec/lib/data_update_scripts/drop_profile_fields_for_static_attributes_spec.rb
View file

@ -1,16 +0,0 @@
require "rails_helper"
require Rails.root.join(
"lib/data_update_scripts/20210630063635_drop_profile_fields_for_static_attributes.rb",
)
describe DataUpdateScripts::DropProfileFieldsForStaticAttributes do
let(:a_group) { create(:profile_field_group) }
it "removes the 3 static profile fields" do
%w[location summary website_url].each do |attribute|
ProfileField.find_or_create_by(attribute_name: attribute, label: attribute, profile_field_group: a_group)
end
expect { described_class.new.run }.to change(ProfileField, :count).by(-3)
end
end

30
spec/lib/data_update_scripts/remove_profile_field_update_scripts_spec.rb Normal file
View file

@ -0,0 +1,30 @@
require "rails_helper"
require Rails.root.join(
"lib/data_update_scripts/20220214195145_remove_profile_field_update_scripts.rb",
)
describe DataUpdateScripts::RemoveProfileFieldUpdateScripts do
it "doesn't raise an error if no DataUpdateScripts are found" do
stub_const "#{described_class}::FILE_NAMES", ["non_existant_data_update_script"]
result = described_class.new.run
expect(result).to eq(0) # delete_by returns 0 if no records are found
end
it "deletes all the unused DataUpdateScripts" do
data_update_script = create(:data_update_script)
stub_const "#{described_class}::FILE_NAMES", [data_update_script.file_name]
expect(DataUpdateScript.find_by(file_name: data_update_script.file_name)).to be_present
described_class.new.run
expect(DataUpdateScript.find_by(file_name: data_update_script.file_name)).not_to be_present
end
it "doesn't delete valid DataUpdateScripts" do
stub_const "#{described_class}::FILE_NAMES", ["non_existant_data_update_script"]
data_update_script = create(:data_update_script)
expect(DataUpdateScript.find_by(file_name: data_update_script.file_name)).to be_present
described_class.new.run
expect(DataUpdateScript.find_by(file_name: data_update_script.file_name)).to be_present
end
end

6
spec/lib/data_update_scripts/set_onboarding_profile_fields_for_existing_forems_spec.rb
View file

@ -11,7 +11,7 @@ describe DataUpdateScripts::SetOnboardingProfileFieldsForExistingForems do
let!(:profile_field1) { create(:profile_field, label: "summary") }
let!(:profile_field2) { create(:profile_field, label: "random") }
let(:profile_field3) { create(:profile_field, label: "location") }
let!(:profile_field3) { create(:profile_field, label: "location") }
it "toggles show_in_onboarding to true for specific profile fields" do
expect do
@ -21,10 +21,6 @@ describe DataUpdateScripts::SetOnboardingProfileFieldsForExistingForems do
end
it "updates the labels for specific profile fields" do
# NOTE: we update the label manually here because when
# we create the profile field the attribute_name is a
# underscored version of the label.
profile_field3.update(label: "Where are you located?")
described_class.new.run
expect(profile_field3.reload.label).to eq("Location")
end

23
spec/lib/data_update_scripts/work_profile_field_follow_up_spec.rb
View file

@ -1,23 +0,0 @@
require "rails_helper"
require Rails.root.join(
"lib/data_update_scripts/20210712044513_work_profile_field_follow_up.rb",
)
describe DataUpdateScripts::WorkProfileFieldFollowUp do
xit "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
xit "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

2
spec/models/profile_field_spec.rb
View file

@ -52,7 +52,7 @@ RSpec.describe ProfileField, type: :model do
describe "callbacks" do
it "automatically generates an attribute name" do
field = create(:profile_field, label: "Test? Test! 1")
expect(field.attribute_name).to eq "test_test1"
expect(field.attribute_name).to start_with("attribute_")
end
describe "#maximum_header_field_count" do

16
spec/models/profile_spec.rb
View file

@ -32,20 +32,23 @@ RSpec.describe Profile, type: :model do
end
describe "validating text areas" do
let(:text_area_get) { ProfileField.find_by(label: "Test Text Area")&.attribute_name }
let(:text_area_set) { "#{text_area_get}=" }
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.test_text_area = "Ruby"
profile.public_send(text_area_set, "Ruby")
expect(profile).to be_valid
end
it "is invalid if the text is too long" do
profile.test_text_area = "x" * ProfileValidator::MAX_TEXT_AREA_LENGTH.next
profile.public_send(text_area_set, "x" * ProfileValidator::MAX_TEXT_AREA_LENGTH.next)
expect(profile).not_to be_valid
expect(profile.errors_as_sentence)
.to eq "Test text area is too long (maximum is 200 characters)"
.to include("is too long (maximum is 200 characters)")
end
end
@ -89,9 +92,12 @@ RSpec.describe Profile, type: :model do
let(:profile) { described_class.new }
let(:test1) { ProfileField.find_by(label: "Test 1").attribute_name }
let(:test2) { ProfileField.find_by(label: "Test 2").attribute_name }
it "defines accessors for active profile fields", :aggregate_failures do
expect(profile).to respond_to(:test1)
expect(profile).to respond_to(:test2)
expect(profile).to respond_to(test1)
expect(profile).to respond_to(test2)
end
end
end

15
spec/requests/profile_preview_cards_spec.rb
View file

@ -50,6 +50,11 @@ RSpec.describe "ProfilePreviewCards", type: :request do
end
let(:profile) { user.profile }
let(:labels_to_attrs) do
%w[Work Education].index_with do |label|
ProfileField.find_by(label: label)&.attribute_name
end
end
context "when signed out" do
it "does not find an unknown user id" do
@ -66,10 +71,11 @@ RSpec.describe "ProfilePreviewCards", type: :request do
get profile_preview_card_path(user), as: :json
preview_card = response.parsed_body
expect(preview_card["summary"]).to eq(profile.summary)
expect(preview_card["work"]).to eq(profile.work)
expect(preview_card["work"]).to eq(profile.public_send(labels_to_attrs["Work"]))
expect(preview_card["location"]).to eq(profile.location)
expect(preview_card["education"]).to eq(profile.education)
expect(preview_card["education"]).to eq(profile.public_send(labels_to_attrs["Education"]))
expect(preview_card["created_at"]).to eq(user.created_at.utc.iso8601)
end
@ -118,10 +124,11 @@ RSpec.describe "ProfilePreviewCards", type: :request do
get profile_preview_card_path(user), as: :json
preview_card = response.parsed_body
expect(preview_card["summary"]).to eq(profile.summary)
expect(preview_card["work"]).to eq(profile.work)
expect(preview_card["work"]).to eq(profile.public_send(labels_to_attrs["Work"]))
expect(preview_card["location"]).to eq(profile.location)
expect(preview_card["education"]).to eq(profile.education)
expect(preview_card["education"]).to eq(profile.public_send(labels_to_attrs["Education"]))
expect(preview_card["created_at"]).to eq(user.created_at.utc.iso8601)
end

5
spec/services/profile_fields/add_spec.rb
View file

@ -6,11 +6,12 @@ RSpec.describe ProfileFields::Add, type: :service do
context "when successfully adding a new profile field" do
it "creates a new profile field and adds a store accessor", :aggregate_failures do
expect(profile.respond_to?(:new_field)).to be false
expect do
described_class.call(label: "New Field", profile_field_group: group)
end.to change(ProfileField, :count).by(1)
expect(profile.respond_to?(:new_field)).to be true
field = ProfileField.find_by(label: "New Field")
expect(profile.respond_to?(field.attribute_name)).to be true
end
it "returns the correct response object", :aggregate_failures do

6
spec/services/profile_fields/remove_spec.rb
View file

@ -5,11 +5,13 @@ RSpec.describe ProfileFields::Remove, type: :service do
it "removes the profile field and store accessor", :aggregate_failures do
profile_field = create(:profile_field, label: "Removed field")
profile = create(:user).profile
getter = profile_field.attribute_name
setter = "#{getter}="
expect { described_class.call(profile_field.id) }
.to change(ProfileField, :count).by(-1)
.and change { profile.respond_to?(:removed_field) }.from(true).to(false)
.and change { profile.respond_to?(:removed_field=) }.from(true).to(false)
.and change { profile.respond_to?(getter) }.from(true).to(false)
.and change { profile.respond_to?(setter) }.from(true).to(false)
end
it "returns the correct response object", :aggregate_failures do

3
spec/services/users/update_spec.rb
View file

@ -44,8 +44,9 @@ RSpec.describe Users::Update, type: :service do
it "updates the profile_updated_at column" do
create(:profile_field, label: "Test field")
attribute_name = ProfileField.find_by(label: "Test field").attribute_name
expect do
described_class.call(user, profile: { test_field: "false" })
described_class.call(user, profile: { attribute_name => "false" })
end.to change { user.reload.profile_updated_at }
end

23
spec/support/seeds/seeds_e2e.rb
View file

@ -29,6 +29,9 @@ ProfileField
.find_or_create_by(label: "Education")
Profile.refresh_attributes!
# extract generated attribute names
work_attr = ProfileField.find_by(label: "Work").attribute_name
education_attr = ProfileField.find_by(label: "Education").attribute_name
##############################################################################
seeder.create_if_doesnt_exist(User, "email", "admin@forem.local") do
@ -52,11 +55,11 @@ seeder.create_if_doesnt_exist(User, "email", "admin@forem.local") do
)
user.profile.update(
summary: "Admin user summary",
work: "Software developer at Company",
location: "Edinburgh",
education: "University of Life",
website_url: Faker::Internet.url,
:summary => "Admin user summary",
work_attr => "Software developer at Company",
:location => "Edinburgh",
education_attr => "University of Life",
:website_url => Faker::Internet.url,
)
user.add_role(:super_admin)
@ -550,11 +553,11 @@ seeder.create_if_doesnt_exist(User, "email", "series-user@forem.local") do
checked_terms_and_conditions: true,
)
series_user.profile.update(
summary: "Series user summary",
work: "Software developer at Company",
location: "Edinburgh",
education: "University of Life",
website_url: Faker::Internet.url,
:summary => "Series user summary",
work_attr => "Software developer at Company",
:location => "Edinburgh",
education_attr => "University of Life",
:website_url => Faker::Internet.url,
)
series_user.notification_setting.update(
email_comment_notifications: false,

8
spec/system/user/user_edits_profile_spec.rb
View file

@ -47,8 +47,8 @@ RSpec.describe "User edits their profile", type: :system do
end
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(left_sidebar_profile_field.label.titleize)
expect(page).to have_text(header_profile_field.label.titleize)
end
it "reflects set profile fields in the interface" do
@ -61,12 +61,12 @@ RSpec.describe "User edits their profile", type: :system do
expect(page).not_to have_text("cthulhu")
within(".crayons-layout__sidebar-left") do
expect(page).to have_text(left_sidebar_profile_field.attribute_name.titleize)
expect(page).to have_text(left_sidebar_profile_field.label.titleize)
expect(page).to have_text("chocolate")
end
within(".profile-header") do
expect(page).to have_text(header_profile_field.attribute_name.titleize)
expect(page).to have_text(header_profile_field.label.titleize)
expect(page).to have_text("pistachio")
end
end

8
spec/workers/moderator/banish_user_worker_spec.rb
View file

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