Merge branch 'add-markdown-support-in-bio' into 'master'

Support markdown in user's bio attribute

See merge request gitlab-org/gitlab!35604

app/assets/javascripts/user_popovers.js

 import Vue from 'vue';
 
+import sanitize from 'sanitize-html';
+
 import UsersCache from './lib/utils/users_cache';
 import UserPopover from './vue_shared/components/user_popover/user_popover.vue';
 
@@ -38,6 +40,7 @@ const populateUserInfo = user => {
           name: userData.name,
           location: userData.location,
           bio: userData.bio,
+          bioHtml: sanitize(userData.bio_html),
           workInformation: userData.work_information,
           loaded: true,
         });

app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue

@@ -75,7 +75,7 @@ export default {
           <div class="gl-text-gray-700">
             <div v-if="user.bio" class="gl-display-flex gl-mb-2">
               <icon name="profile" class="gl-text-gray-600 gl-flex-shrink-0" />
-              <span ref="bio" class="gl-ml-2">{{ user.bio }}</span>
+              <span ref="bio" class="ml-1" v-html="user.bioHtml"></span>
             </div>
             <div v-if="user.workInformation" class="gl-display-flex gl-mb-2">
               <icon name="work" class="gl-text-gray-600 gl-flex-shrink-0" />

app/models/user.rb

@@ -69,6 +69,8 @@ class User < ApplicationRecord
 
   MINIMUM_INACTIVE_DAYS = 180
 
+  ignore_column :bio, remove_with: '13.4', remove_after: '2020-09-22'
+
   # Override Devise::Models::Trackable#update_tracked_fields!
   # to limit database writes to at most once every hour
   # rubocop: disable CodeReuse/ServiceClass
@@ -193,7 +195,6 @@ class User < ApplicationRecord
   validates :notification_email, devise_email: true, if: ->(user) { user.notification_email != user.email }
   validates :public_email, presence: true, uniqueness: true, devise_email: true, allow_blank: true
   validates :commit_email, devise_email: true, allow_nil: true, if: ->(user) { user.commit_email != user.email }
-  validates :bio, length: { maximum: 255 }, allow_blank: true
   validates :projects_limit,
     presence: true,
     numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE }
@@ -228,7 +229,6 @@ class User < ApplicationRecord
   before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? }
   before_validation :ensure_namespace_correct
   before_save :ensure_namespace_correct # in case validation is skipped
-  before_save :ensure_bio_is_assigned_to_user_details, if: :bio_changed?
   after_validation :set_username_errors
   after_update :username_changed_hook, if: :saved_change_to_username?
   after_destroy :post_destroy_hook
@@ -280,6 +280,7 @@ class User < ApplicationRecord
 
   delegate :path, to: :namespace, allow_nil: true, prefix: true
   delegate :job_title, :job_title=, to: :user_detail, allow_nil: true
+  delegate :bio, :bio=, :bio_html, to: :user_detail, allow_nil: true
 
   accepts_nested_attributes_for :user_preference, update_only: true
   accepts_nested_attributes_for :user_detail, update_only: true
@@ -1272,13 +1273,6 @@ class User < ApplicationRecord
     end
   end
 
-  # Temporary, will be removed when bio is fully migrated
-  def ensure_bio_is_assigned_to_user_details
-    return if Feature.disabled?(:migrate_bio_to_user_details, default_enabled: true)
-
-    user_detail.bio = bio.to_s[0...255] # bio can be NULL in users, but cannot be NULL in user_details
-  end
-
   def set_username_errors
     namespace_path_errors = self.errors.delete(:"namespace.path")
     self.errors[:username].concat(namespace_path_errors) if namespace_path_errors

app/models/user_detail.rb

 # frozen_string_literal: true
 
 class UserDetail < ApplicationRecord
+  extend ::Gitlab::Utils::Override
+  include CacheMarkdownField
+
   belongs_to :user
 
   validates :job_title, length: { maximum: 200 }
+  validates :bio, length: { maximum: 255 }, allow_blank: true
+
+  cache_markdown_field :bio
+
+  def bio_html
+    read_attribute(:bio_html) || bio
+  end
+
+  # For backward compatibility.
+  # Older migrations (and their tests) reference the `User.migration_bot` where the `bio` attribute is set.
+  # Here we disable writing the markdown cache when the `bio_html` column does not exists.
+  override :invalidated_markdown_cache?
+  def invalidated_markdown_cache?
+    self.class.column_names.include?('bio_html') && super
+  end
 end

app/views/users/show.html.haml

@@ -85,7 +85,8 @@
         - if @user.bio.present?
           .cover-desc.cgray
             %p.profile-user-bio
-              = @user.bio
+              = markdown(@user.bio_html)
+
 
     - unless profile_tabs.empty?
       .scrolling-tabs-container

changelogs/unreleased/add-markdown-support-in-bio.yml

+---
+title: Add support for Markdown in the user's bio
+merge_request: 35604
+author: Riccardo Padovani
+type: added

config/locales/en.yml

@@ -22,6 +22,8 @@ en:
         grafana_enabled: "Grafana integration enabled"
       user/user_detail:
         job_title: 'Job title'
+      user/user_detail:
+        bio: 'Bio'
   views:
     pagination:
       previous: "Prev"

db/migrate/20200630091656_add_bio_html_to_user_details.rb

+# frozen_string_literal: true
+
+class AddBioHtmlToUserDetails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    with_lock_retries do
+      # Note: bio_html is calculated from bio, the bio column is already constrained
+      add_column :user_details, :bio_html, :text # rubocop:disable Migration/AddLimitToTextColumns
+      add_column :user_details, :cached_markdown_version, :integer
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :user_details, :bio_html
+      remove_column :user_details, :cached_markdown_version
+    end
+  end
+end

db/structure.sql

@@ -15640,7 +15640,9 @@ ALTER SEQUENCE public.user_custom_attributes_id_seq OWNED BY public.user_custom_
 CREATE TABLE public.user_details (
     user_id bigint NOT NULL,
     job_title character varying(200) DEFAULT ''::character varying NOT NULL,
-    bio character varying(255) DEFAULT ''::character varying NOT NULL
+    bio character varying(255) DEFAULT ''::character varying NOT NULL,
+    bio_html text,
+    cached_markdown_version integer
 );
 
 CREATE SEQUENCE public.user_details_user_id_seq
@@ -23635,6 +23637,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200626060151
 20200626130220
 20200629192638
+20200630091656
 20200630110826
 20200701093859
 20200702123805

doc/api/users.md

@@ -90,6 +90,7 @@ GET /users
     "created_at": "2012-05-23T08:00:58Z",
     "is_admin": false,
     "bio": null,
+    "bio_html": null,
     "location": null,
     "skype": "",
     "linkedin": "",
@@ -129,6 +130,7 @@ GET /users
     "created_at": "2012-05-23T08:01:01Z",
     "is_admin": false,
     "bio": null,
+    "bio_html": null,
     "location": null,
     "skype": "",
     "linkedin": "",
@@ -246,6 +248,7 @@ Parameters:
   "web_url": "http://localhost:3000/john_smith",
   "created_at": "2012-05-23T08:00:58Z",
   "bio": null,
+  "bio_html": null,
   "location": null,
   "public_email": "john@example.com",
   "skype": "",
@@ -281,6 +284,7 @@ Example Responses:
   "created_at": "2012-05-23T08:00:58Z",
   "is_admin": false,
   "bio": null,
+  "bio_html": null,
   "location": null,
   "public_email": "john@example.com",
   "skype": "",
@@ -500,6 +504,7 @@ GET /user
   "web_url": "http://localhost:3000/john_smith",
   "created_at": "2012-05-23T08:00:58Z",
   "bio": null,
+  "bio_html": null,
   "location": null,
   "public_email": "john@example.com",
   "skype": "",
@@ -549,6 +554,7 @@ GET /user
   "created_at": "2012-05-23T08:00:58Z",
   "is_admin": false,
   "bio": null,
+  "bio_html": null,
   "location": null,
   "public_email": "john@example.com",
   "skype": "",

lib/api/entities/user.rb

@@ -5,7 +5,7 @@ module API
     class User < UserBasic
       include UsersHelper
       expose :created_at, if: ->(user, opts) { Ability.allowed?(opts[:current_user], :read_user_profile, user) }
-      expose :bio, :location, :public_email, :skype, :linkedin, :twitter, :website_url, :organization, :job_title
+      expose :bio, :bio_html, :location, :public_email, :skype, :linkedin, :twitter, :website_url, :organization, :job_title
       expose :work_information do |user|
         work_information(user)
       end

lib/api/helpers.rb

@@ -410,10 +410,14 @@ module API
 
     def render_validation_error!(model)
       if model.errors.any?
-        render_api_error!(model.errors.messages || '400 Bad Request', 400)
+        render_api_error!(model_error_messages(model) || '400 Bad Request', 400)
       end
     end
 
+    def model_error_messages(model)
+      model.errors.messages
+    end
+
     def render_spam_error!
       render_api_error!({ error: 'Spam detected' }, 400)
     end

lib/api/helpers/users_helpers.rb

@@ -11,6 +11,13 @@ module API
 
       params :optional_index_params_ee do
       end
+
+      def model_error_messages(model)
+        super.tap do |error_messages|
+          # Remapping errors from nested associations.
+          error_messages[:bio] = error_messages.delete(:"user_detail.bio") if error_messages.has_key?(:"user_detail.bio")
+        end
+      end
     end
   end
 end

lib/api/users.rb

@@ -117,6 +117,8 @@ module API
         users = users.preload(:identities, :u2f_registrations) if entity == Entities::UserWithAdmin
         users, options = with_custom_attributes(users, { with: entity, current_user: current_user })
 
+        users = users.preload(:user_detail)
+
         present paginate(users), options
       end
       # rubocop: enable CodeReuse/ActiveRecord

spec/features/profiles/user_edit_profile_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe 'User edit profile' do
     fill_in 'user_twitter', with: 'testtwitter'
     fill_in 'user_website_url', with: 'testurl'
     fill_in 'user_location', with: 'Ukraine'
-    fill_in 'user_bio', with: 'I <3 GitLab'
+    fill_in 'user_bio', with: 'I <3 GitLab :tada:'
     fill_in 'user_job_title', with: 'Frontend Engineer'
     fill_in 'user_organization', with: 'GitLab'
     submit_settings
@@ -36,7 +36,8 @@ RSpec.describe 'User edit profile' do
       linkedin: 'testlinkedin',
       twitter: 'testtwitter',
       website_url: 'testurl',
-      bio: 'I <3 GitLab',
+      bio: 'I <3 GitLab :tada:',
+      bio_html: '<p data-sourcepos="1:1-1:18" dir="auto">I &lt;3 GitLab <gl-emoji title="party popper" data-name="tada" data-unicode-version="6.0">🎉</gl-emoji></p>',
       job_title: 'Frontend Engineer',
       organization: 'GitLab'
     )

spec/frontend/vue_shared/components/user_popover/user_popover_spec.js

@@ -83,9 +83,10 @@ describe('User Popover Component', () => {
   describe('job data', () => {
     const findWorkInformation = () => wrapper.find({ ref: 'workInformation' });
     const findBio = () => wrapper.find({ ref: 'bio' });
+    const bio = 'My super interesting bio';
 
     it('should show only bio if work information is not available', () => {
-      const user = { ...DEFAULT_PROPS.user, bio: 'My super interesting bio' };
+      const user = { ...DEFAULT_PROPS.user, bio, bioHtml: bio };
 
       createWrapper({ user });
 
@@ -107,7 +108,8 @@ describe('User Popover Component', () => {
     it('should display bio and work information in separate lines', () => {
       const user = {
         ...DEFAULT_PROPS.user,
-        bio: 'My super interesting bio',
+        bio,
+        bioHtml: bio,
         workInformation: 'Frontend Engineer at GitLab',
       };
 
@@ -120,12 +122,13 @@ describe('User Popover Component', () => {
     it('should not encode special characters in bio', () => {
       const user = {
         ...DEFAULT_PROPS.user,
-        bio: 'I like <html> & CSS',
+        bio: 'I like CSS',
+        bioHtml: 'I like <b>CSS</b>',
       };
 
       createWrapper({ user });
 
-      expect(findBio().text()).toBe('I like <html> & CSS');
+      expect(findBio().html()).toContain('I like <b>CSS</b>');
     });
 
     it('shows icon for bio', () => {

spec/models/user_detail_spec.rb

@@ -6,9 +6,38 @@ RSpec.describe UserDetail do
   it { is_expected.to belong_to(:user) }
 
   describe 'validations' do
-    describe 'job_title' do
+    describe '#job_title' do
       it { is_expected.not_to validate_presence_of(:job_title) }
       it { is_expected.to validate_length_of(:job_title).is_at_most(200) }
     end
+
+    describe '#bio' do
+      it { is_expected.to validate_length_of(:bio).is_at_most(255) }
+    end
+  end
+
+  describe '#bio_html' do
+    let(:user) { create(:user, bio: 'some **bio**') }
+
+    subject { user.user_detail.bio_html }
+
+    it 'falls back to #bio when the html representation is missing' do
+      user.user_detail.update!(bio_html: nil)
+
+      expect(subject).to eq(user.user_detail.bio)
+    end
+
+    it 'stores rendered html' do
+      expect(subject).to include('some <strong>bio</strong>')
+    end
+
+    it 'does not try to set the value when the column is not there' do
+      without_bio_html_column = UserDetail.column_names - ['bio_html']
+
+      expect(described_class).to receive(:column_names).at_least(:once).and_return(without_bio_html_column)
+      expect(user.user_detail).not_to receive(:bio_html=)
+
+      subject
+    end
   end
 end

spec/models/user_spec.rb

@@ -58,6 +58,10 @@ RSpec.describe User do
 
     it { is_expected.to delegate_method(:job_title).to(:user_detail).allow_nil }
     it { is_expected.to delegate_method(:job_title=).to(:user_detail).with_arguments(:args).allow_nil }
+
+    it { is_expected.to delegate_method(:bio).to(:user_detail).allow_nil }
+    it { is_expected.to delegate_method(:bio=).to(:user_detail).with_arguments(:args).allow_nil }
+    it { is_expected.to delegate_method(:bio_html).to(:user_detail).allow_nil }
   end
 
   describe 'associations' do
@@ -91,64 +95,28 @@ RSpec.describe User do
     it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:user) }
     it { is_expected.to have_many(:reviews).inverse_of(:author) }
 
-    describe "#bio" do
-      it 'syncs bio with `user_details.bio` on create' do
-        user = create(:user, bio: 'my bio')
-
-        expect(user.bio).to eq(user.user_detail.bio)
-      end
-
-      context 'when `migrate_bio_to_user_details` feature flag is off' do
-        before do
-          stub_feature_flags(migrate_bio_to_user_details: false)
+    describe "#user_detail" do
+      it 'does not persist `user_detail` by default' do
+        expect(create(:user).user_detail).not_to be_persisted
       end
 
-        it 'does not sync bio with `user_details.bio`' do
+      it 'creates `user_detail` when `bio` is given' do
         user = create(:user, bio: 'my bio')
 
-          expect(user.bio).to eq('my bio')
-          expect(user.user_detail.bio).to eq('')
-        end
-      end
-
-      it 'syncs bio with `user_details.bio` on update' do
-        user = create(:user)
-
-        user.update!(bio: 'my bio')
-
-        expect(user.bio).to eq(user.user_detail.bio)
+        expect(user.user_detail).to be_persisted
+        expect(user.user_detail.bio).to eq('my bio')
       end
 
-      context 'when `user_details` association already exists' do
-        let(:user) { create(:user) }
-
-        before do
-          create(:user_detail, user: user)
-        end
-
-        it 'syncs bio with `user_details.bio`' do
-          user.update!(bio: 'my bio')
+      it 'delegates `bio` to `user_detail`' do
+        user = create(:user, bio: 'my bio')
 
         expect(user.bio).to eq(user.user_detail.bio)
       end
 
-        it 'falls back to "" when nil is given' do
-          user.update!(bio: nil)
-
-          expect(user.bio).to eq(nil)
-          expect(user.user_detail.bio).to eq('')
-        end
-
-        # very unlikely scenario
-        it 'truncates long bio when syncing to user_details' do
-          invalid_bio = 'a' * 256
-          truncated_bio = 'a' * 255
-
-          user.bio = invalid_bio
-          user.save(validate: false)
+      it 'creates `user_detail` when `bio` is first updated' do
+        user = create(:user)
 
-          expect(user.user_detail.bio).to eq(truncated_bio)
-        end
+        expect { user.update(bio: 'my bio') }.to change { user.user_detail.persisted? }.from(false).to(true)
       end
     end
 
@@ -337,8 +305,6 @@ RSpec.describe User do
     it { is_expected.not_to allow_value(-1).for(:projects_limit) }
     it { is_expected.not_to allow_value(Gitlab::Database::MAX_INT_VALUE + 1).for(:projects_limit) }
 
-    it { is_expected.to validate_length_of(:bio).is_at_most(255) }
-
     it_behaves_like 'an object with email-formated attributes', :email do
       subject { build(:user) }
     end