Add bluesky profile field
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 |
---|---|
![]() |
![]() ![]() |
The user profile side contact info
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
- Add proper did:plc to your user profile (see https://atproto.com/specs/did)
- Go to the user profile
Related to #451690 (closed)
Merge request reports
Activity
mentioned in issue #451690 (closed)
Hey @SlickDomique!
Thank you for your contribution to GitLab. Please refer to the contribution documentation for an overview of the process.
Did you know about our community forks? Working from there will make your contribution process easier. Please check it out!
When you're ready for a first review, post
@gitlab-bot ready
. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with@gitlab-bot ready @user1 @user2
.At any time, if you need help, feel free to post
@gitlab-bot help
or initiate a mentor session on Discord. Read more on how to get help.You can comment
@gitlab-bot label <label1> <label2>
to add labels to your MR. Please see the list of allowed labels in thelabel
command documentation.This message was generated automatically. You're welcome to improve it.
added Community contribution workflowin dev labels
assigned to @SlickDomique
@csouthard @kniechajewicz @s_awezec this merge request touches files that could potentially affect user growth or subscription cost management.
This message was generated automatically. You're welcome to improve it.
added linked-issue label
mentioned in issue gitlab-org/quality/triage-reports#16909 (closed)
added Category:User Profile featureaddition grouptenant scale typefeature labels
added devopsdata stores sectioncore platform labels
added UX label
Thanks for helping us improve the UX of GitLab. Your contribution is appreciated! We have pinged our UX team, so stay tuned for their feedback.
This message was generated automatically. You're welcome to improve it.
added groupauthentication label and removed grouptenant scale label
added devopsgovern sectionsec labels and removed devopsdata stores sectioncore platform labels
- Resolved by Domi
It seems that CI pipeline failure is related to the changes in this MR. Do you mind checking the failed jobs?
Since the MR was assigned to groupauthentication (see why) feel free to ping folks from that team or @adil.farrukh for further guidance
We also need review from database and Technical Writing.
added Technical Writing database labels
requested review from @mle
@SlickDomique LGTM Thank you for the contribution. I checked it for functionality from the UX.
removed review request for @mle
added pipeline:mr-approved label
added pipeline:mr-approved label
- Resolved by Domi
@mle
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break
master
, please start a new pipeline before merging.Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.
changed milestone to %16.11
mentioned in merge request gitlab-com/www-gitlab-com!133684 (merged)
- Resolved by Domi
Thanks @SlickDomique happy to approve the documentation change.
- Resolved by Domi
- Resolved by Domi
- Resolved by Brian Williams
- Resolved by Domi
- Resolved by Domi
- Resolved by Domi
- Resolved by Domi
Thanks for your contribution @SlickDomique. Since there's a migration we need a database review. I'm assigning someone.
I also requested a few minor changes.
requested review from @bwill
- Resolved by Domi
- Resolved by Domi
removed review request for @bwill
added 1 commit
- 4655f5a6 - MR suggestions for user_detail model, cleaning up structure.sql
changed milestone to %17.0
@SlickDomique Do you have any thoughts on addressing this piece of feedback? I'm hoping we can get this into our next release, looking forward to the feature
@hsutor Not sure what I should do, the comment said it's a mention towards the team, not a remark for me. I haven't got any response to my proposal for this too
@hsutor That comment does not need to be addressed in this MR. I will have a look today to see if this can be merged.
@hsutor @SlickDomique Looking over this MR, we still need reviews for frontend and database maintainer. Myself and Drew have both reviewed this MR for backend. It looks good to me but I am going to ask Drew to take another look since there are new changes since his last review.
Below is the current state of approvals
Role Reviewer Maintainer UX @mle
Not required Technical Writing @jglassman1
Not required database @bwill
Needs review frontend Needs review Needs review backend @bwill
@dblessing
Pending@tigerwnz Can you please review as database maintainer?
@dblessing Can you please review as backend maintainer?
@robyrne Can you please review as frontend reviewer?I've just realized that Drew is on leave. @dmeshcharakou Could you give this the final look-over for backend please?
@SlickDomique Nice work, frontend looks good. I left one suggestion for bumping the milestone version in the migration but I'll pass it to the maintainer.
@dpisek could you take the frontend maintainer review, please?
Thanks @SlickDomique!
backend looks great
I left a few suggestions for the tests. Would you mind to have a look and let me know what you think?
Nice work @SlickDomique, thanks for the contribution
I left a comment, but it is not blocking and anything that comes out of it can be handled in a separate effort
@SlickDomique I'll unassign myself for now, but please feel free to assign me as a reviewer again when the feedback is addressed. You can use the following command:
@gitlab-bot ready @dmeshcharakou
requested review from @bwill
- A deleted user
added backend databasereview pending documentation frontend labels
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, Did you consider new validations can break existing records? Please follow the code quality guidelines about new model validations when adding a new model validation.
If you're adding the validations to a model with no records you can ignore this warning.
4 Warnings 61043bb4: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 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. 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?
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 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.
This merge request adds or changes files that require a review from the Database team. 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:
- Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
- Prepare your MR for database review according to the docs.
- 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:
-
doc/user/profile/index.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Category Reviewer Maintainer backend @dfrazao-gitlab
(UTC+2)
@georgekoltsov
(UTC+1)
database @jdrpereira
(UTC+1)
@tigerwnz
(UTC+10)
frontend @robyrne
(UTC+2)
@sdejonge
(UTC+10)
UX @ipelaez1
(UTC-4)
Maintainer review is optional for UX Please check reviewer's status!
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
danger-review
job that generated this comment.Generated by
DangerThe 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.
- It doesn't work for the
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 361ed11e and 4655f5a6
Special assetsEntrypoint / 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
Dangeradded databasereviewed label and removed databasereview pending label
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:
test report for 4655f5a6expand 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:
test report for 4655f5a6expand 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 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
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' The milestone needs to be updated to the current one https://gitlab.com/groups/gitlab-org/-/milestones/87#tab-issues
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
- Resolved by David Pisek
351 351 */ 352 352 $discord: #5865f2; 353 353 $linkedin: #2867b2; 354 $bluesky: #1185fe; Given the comment below, we won't need this
removed review request for @dmeshcharakou
changed milestone to %17.1
@SlickDomique, it seems we're waiting on an action from you for approximately two weeks.
- Do you still have capacity to work on this? If not, you might want to close this MR and/or ask someone to take over.
- Do you need help in getting it ready? At any time, you can:
- If you're actually ready for a review, you can post
@gitlab-bot ready
.
This message was generated automatically. You're welcome to improve it.
added automation:author-reminded label
added idle label
@SlickDomique
Just a friendly check if you need any help addressing the review feedback?@SlickDomique No worries at all! I'll ask an groupauthentication to try continue this work. If/when they do, they will link the MR here and ensure to give you credit once it's completed
removed idle label
changed milestone to %17.3
added pipelinetier-2 label
removed review request for @tigerwnz
assigned to @eduardosanz
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:
- React with a
or a on this comment to describe your experience. - 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!
This message was generated automatically. You're welcome to improve it.
- React with a
Hello @SlickDomique
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!
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)