Skip to content
Snippets Groups Projects

Add bluesky profile field

Closed Domi requested to merge SlickDomique/gitlab:451690-bluesky-profile-field into master
11 unresolved threads

What does this MR do and why?

Adds a profile field for bluesky did:plc identifier

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After
image image image

The user profile side contact info

image

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Add proper did:plc to your user profile (see https://atproto.com/specs/did)
  2. Go to the user profile

Related to #451690 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Domi added 1 commit

    added 1 commit

    • 8c07d6e9 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Jon Glassman approved this merge request

    approved this merge request

  • Thanks @SlickDomique happy to approve the documentation change.

  • Aboobacker MK
  • Drew Blessing
  • Drew Blessing
  • Drew Blessing
  • Drew Blessing
  • Drew Blessing
  • Drew Blessing requested review from @bwill

    requested review from @bwill

  • Brian Williams
  • Brian Williams
  • Brian Williams removed review request for @bwill

    removed review request for @bwill

  • Domi added 1 commit

    added 1 commit

    • 4655f5a6 - MR suggestions for user_detail model, cleaning up structure.sql

    Compare with previous version

  • Hannah Sutor changed milestone to %17.0

    changed milestone to %17.0

  • Brian Williams requested review from @bwill

    requested review from @bwill

  • Ghost User
  • 34 42 validate :discord_format
    35 43 validates :linkedin, length: { maximum: DEFAULT_FIELD_LENGTH }, allow_blank: true
    36 44 validates :location, length: { maximum: DEFAULT_FIELD_LENGTH }, allow_blank: true
    45 validates :bluesky,
    • 4 Warnings
      :warning: 61043bb4: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
      :warning: 61043bb4: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
      :warning: :hourglass: Migration Timestamp Out of Date
      The following migrations have timestamps that are over three weeks old:
      • db/migrate/20240324133741_add_bluesky_to_user_details.rb

      Please double check the timestamps and update them if possible. Why does this matter?

      :warning: The master pipeline status page reported failures in

      If these jobs fail in your merge request with the same errors, then they are not caused by your changes.
      Please check for any on-going incidents in the incident issue tracker or in the #master-broken Slack channel.

      3 Messages
      :book: CHANGELOG missing:

      If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

      If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

      :book: This merge request adds or changes files that require a review from the Database team.
      :book: This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge.

      This merge request requires a database review. To make sure these changes are reviewed, take the following steps:

      1. Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
      2. Prepare your MR for database review according to the docs.
      3. Assign and mention the database reviewer suggested by Reviewer Roulette.

      The following files require a review from the Database team:

      • db/migrate/20240324133741_add_bluesky_to_user_details.rb
      • db/schema_migrations/20240324133741
      • db/structure.sql

      Documentation review

      The following files require a review from a technical writer:

      The review does not need to block merging this merge request. See the:

      Reviewer roulette

      Category Reviewer Maintainer
      backend @dfrazao-gitlab profile link current availability (UTC+2) @georgekoltsov profile link current availability (UTC+1)
      database @jdrpereira profile link current availability (UTC+1) @tigerwnz profile link current availability (UTC+10)
      frontend @robyrne profile link current availability (UTC+2) @sdejonge profile link current availability (UTC+10)
      UX @ipelaez1 profile link current availability (UTC-4) Maintainer review is optional for UX

      Please check reviewer's status!

      • available Reviewer is available!
      • unavailable Reviewer is unavailable!

      Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

      Tailwind CSS

      Legacy utils

      The following files contain legacy utils:

      • app/views/users/_profile_sidebar.html.haml
        • gl-display-flex

      Use the Tailwind documentation to find the Tailwind equivalent to these legacy utils. If the Tailwind equivalent is not available it is okay to use the legacy util for now. The Tailwind equivalent will be made available when the corresponding migration issue in &13521 (closed) is completed.

      If needed, you can retry the :repeat: danger-review job that generated this comment.

      Generated by :no_entry_sign: Danger

    • The following migrations have timestamps that are over three weeks old:

      • db/migrate/20240324133741_add_bluesky_to_user_details.rb

      Please double check the timestamps and update them if possible. Why does this matter?

      @SlickDomique Can we please address this? We can do this by changing the timestamps in the file names to a more recent one like 20240502134352.

      mv db/migrate/{20240324133741,20240502134352}_add_bluesky_to_user_details.rb
      mv db/schema_migrations/{20240324133741,20240502134352}
    • We can do this by changing the timestamps in the file names to a more recent one like 20240502134352

      It is a good idea to update the timestamps, but there's a couple of things to be wary of when doing it this way (renaming files):

      • It doesn't work for the db/schema_migrations digest file, as the digest is generated using the migration timestamp. You'd need to re-migrate to regenerate these files in this case (which will remove the file for the old timestamp and add a new one).
      • Accidentally picking a timestamp in the future causes problems due to how Rails handles future values (it increments them by one when generating the next migration, which means branches will create migrations with conflicting timestamps).

      It's best to follow the steps listed in the docs and rollback/recreate the migration.

    • Please register or sign in to reply
  • Brian Williams approved this merge request

    approved this merge request

  • Brian Williams changed title from adding bluesky profile field to Add bluesky profile field

    changed title from adding bluesky profile field to Add bluesky profile field

  • Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 361ed11e and 4655f5a6

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 4.31 MB 4.31 MB - 0.0 %
    mainChunk 3.3 MB 3.3 MB - 0.0 %

    Note: We do not have exact data for 361ed11e. So we have used data from: 964afd3a.
    The intended commit has no webpack pipeline, so we chose the last commit with one before it.

    Please look at the full report for more details


    Read more about how this report works.

    Generated by :no_entry_sign: Danger

  • Brian Williams requested review from @tigerwnz, @dblessing, and @robyrne and removed review request for @bwill

    requested review from @tigerwnz, @dblessing, and @robyrne and removed review request for @bwill

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for 4655f5a6

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Create      | 96     | 0      | 9       | 0     | 105   | ✅     |
    | Data Stores | 27     | 0      | 4       | 0     | 31    | ✅     |
    | Govern      | 66     | 0      | 0       | 0     | 66    | ✅     |
    | Plan        | 51     | 0      | 2       | 0     | 53    | ✅     |
    | Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
    | Package     | 19     | 0      | 12      | 0     | 31    | ✅     |
    | Verify      | 35     | 0      | 1       | 0     | 36    | ✅     |
    | Monitor     | 7      | 0      | 0       | 0     | 7     | ✅     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 308    | 0      | 29      | 0     | 337   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :white_check_mark: test report for 4655f5a6

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Govern      | 300    | 0      | 13      | 12    | 313   | ✅     |
    | Create      | 182    | 0      | 21      | 0     | 203   | ✅     |
    | Data Stores | 14     | 0      | 8       | 0     | 22    | ✅     |
    | Plan        | 44     | 0      | 4       | 0     | 48    | ✅     |
    | Package     | 6      | 0      | 8       | 0     | 14    | ✅     |
    | Verify      | 18     | 0      | 0       | 0     | 18    | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Release     | 2      | 0      | 0       | 0     | 2     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 574    | 0      | 54      | 12    | 628   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • Brian Williams requested review from @dmeshcharakou and removed review request for @dblessing

    requested review from @dmeshcharakou and removed review request for @dblessing

  • 1 # frozen_string_literal: true
    2
    3 class AddBlueskyToUserDetails < Gitlab::Database::Migration[2.2]
    4 disable_ddl_transaction!
    5 milestone '16.11'
  • Ross Byrne approved this merge request

    approved this merge request

  • Ross Byrne requested review from @dpisek and removed review request for @robyrne

    requested review from @dpisek and removed review request for @robyrne

  • 1 # frozen_string_literal: true
    2
    3 class AddBlueskyToUserDetails < Gitlab::Database::Migration[2.2]
    4 disable_ddl_transaction!
    5 milestone '16.11'
  • 668 668 end
    669 669 end
    670 670
    671 context 'when bluesky is set' do
    672 let_it_be(:user) { build(:user) }
  • 159 159 end
    160 160 end
    161 161
    162 describe '#bluesky' do
    163 context 'when bluesky is set' do
    164 let_it_be(:user_detail) { create(:user_detail) }
  • 159 159 end
    160 160 end
    161 161
    162 describe '#bluesky' do
    163 context 'when bluesky is set' do
    164 let_it_be(:user_detail) { create(:user_detail) }
    165
    166 it 'accepts a valid bluesky did id' do
    167 user_detail.bluesky = 'did:plc:ewvi7nxzyoun6zhxrhs64oiz'
    168
    169 expect(user_detail).to be_valid
    170 end
    171
    172 context 'when bluesky is set to a wrong format' do
    • What do you think about slightly DRY-ing the tests?

      diff --git a/spec/models/user_detail_spec.rb b/spec/models/user_detail_spec.rb
      index 997c89f9af21..f819a9659084 100644
      --- a/spec/models/user_detail_spec.rb
      +++ b/spec/models/user_detail_spec.rb
      @@ -126,37 +126,43 @@
       
           describe '#bluesky' do
             context 'when bluesky is set' do
      -        let_it_be(:user_detail) { create(:user_detail) }
      +        let_it_be(:user_detail) { build(:user_detail) }
       
      -        it 'accepts a valid bluesky did id' do
      -          user_detail.bluesky = 'did:plc:ewvi7nxzyoun6zhxrhs64oiz'
      +        let(:value) { 'did:plc:ewvi7nxzyoun6zhxrhs64oiz' }
       
      -          expect(user_detail).to be_valid
      +        before do
      +          user_detail.bluesky = value
               end
       
      -        context 'when bluesky is set to a wrong format' do
      -          it 'throws an error when bluesky did:plc is too long' do
      -            user_detail.bluesky = 'a' * 33
      +        it 'accepts a valid bluesky did id' do
      +          expect(user_detail).to be_valid
      +        end
       
      +        shared_examples 'throws an error', :aggregate_failures do
      +          it do
                   expect(user_detail).not_to be_valid
                   expect(user_detail.errors.full_messages)
                     .to match_array([_('Bluesky must contain only a bluesky did:plc identifier.')])
                 end
      +        end
      +
      +        context 'when bluesky is set to a wrong format' do
      +          context 'when bluesky did:plc is too long' do
      +            let(:value) { 'a' * 33 }
      +
      +            it_behaves_like 'throws an error'
      +          end
       
      -          it 'throws an error when bluesky did:plc is wrong' do
      -            user_detail.bluesky = 'did:plc:ewvi7nxzyoun6zhxrhs64OIZ'
      +          context 'when bluesky did:plc is wrong' do
      +            let(:value) { 'did:plc:ewvi7nxzyoun6zhxrhs64OIZ' }
       
      -            expect(user_detail).not_to be_valid
      -            expect(user_detail.errors.full_messages)
      -              .to match_array([_('Bluesky must contain only a bluesky did:plc identifier.')])
      +            it_behaves_like 'throws an error'
                 end
       
      -          it 'throws an error when bluesky other bluesky did: formats are used' do
      -            user_detail.bluesky = 'did:web:example.com'
      +          context 'when bluesky other bluesky did: formats are used' do
      +            let(:value) { 'did:web:example.com' }
       
      -            expect(user_detail).not_to be_valid
      -            expect(user_detail.errors.full_messages)
      -              .to match_array([_('Bluesky must contain only a bluesky did:plc identifier.')])
      +            it_behaves_like 'throws an error'
                 end
               end
             end
    • Please register or sign in to reply
  • David Pisek
  • David Pisek approved this merge request

    approved this merge request

  • 351 351 */
    352 352 $discord: #5865f2;
    353 353 $linkedin: #2867b2;
    354 $bluesky: #1185fe;
  • 133 133 color: $discord;
    134 134 }
    135 135
    136 .bluesky-icon {
    137 color: $bluesky;
    138 }
  • removed review request for @dmeshcharakou

  • Adil Farrukh changed milestone to %17.1

    changed milestone to %17.1

  • @SlickDomique, it seems we're waiting on an action from you for approximately two weeks.

    This message was generated automatically. You're welcome to improve it.

  • added idle label

  • removed idle label

  • Adil Farrukh changed milestone to %17.3

    changed milestone to %17.3

  • Tiger Watson removed review request for @tigerwnz

    removed review request for @tigerwnz

  • added idle label

  • @SlickDomique, I have been asked to move this MR forward. I see there are quite a bit of conflicts. If you don't mind I am going to close this MR and reopen a new one. You will be properly attributed.

    Thank you very much!

  • @SlickDomique, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

    1. React with a :thumbsup: or a :thumbsdown: on this comment to describe your experience.
    2. Create a new comment starting with @gitlab-bot feedback below, and leave any additional feedback you have for us in the comment.

    Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.

    Thanks for your help! :heart:

    This message was generated automatically. You're welcome to improve it.

  • Hello @SlickDomique :wave:

    The database team is looking for ways to improve the database review process and we would love your help!

    If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:

    @gitlab-org/database-team

    And someone will be by shortly!

    Thanks for your help! :heart:

    This message was generated automatically. You're welcome to improve it.

  • removed idle label

  • mentioned in merge request !161228 (merged)

  • The MR was continued here: !161228 (merged)

  • Please register or sign in to reply
    Loading