-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-10365] Drop OrganizationUser AccessAll #4701
[PM-10365] Drop OrganizationUser AccessAll #4701
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4701 +/- ##
==========================================
- Coverage 41.78% 41.78% -0.01%
==========================================
Files 1308 1308
Lines 62131 62128 -3
Branches 5725 5725
==========================================
- Hits 25964 25961 -3
Misses 34972 34972
Partials 1195 1195 ☔ View full report in Codecov by Sentry. |
New Issues
Fixed Issues
|
f73c672
to
94a400a
Compare
-- Recreate the index without the column | ||
PRINT N'Recreating index IX_OrganizationUser_UserIdOrganizationIdStatus...'; | ||
CREATE NONCLUSTERED INDEX [IX_OrganizationUser_UserIdOrganizationIdStatus] | ||
ON [dbo].[OrganizationUser]([UserId] ASC, [OrganizationId] ASC, [Status] ASC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the index rebuild
IF OBJECT_ID('[dbo].[DF_OrganizationUser_AccessAll]', 'D') IS NULL | ||
AND COL_LENGTH('[dbo].[OrganizationUser]', 'AccessAll') IS NOT NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rkac-bw the equivalent migraiton for Groups
didn't check for the existence of the column (only for the non-existence of the constraint), so it throws an error if run multiple times. Theoretically dbup shouldn't run a migration multiple times, but it is something we try to support. Do you want me to go back and fix that?
…l-from-sprocs-and-table
…l-from-sprocs-and-table
…l-from-sprocs-and-table
…l-from-sprocs-and-table
…l-from-sprocs-and-table
c9850da
to
93bbc0b
Compare
…l-from-sprocs-and-table
…ganizationuser-accessall-from-sprocs-and-table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-10365
📔 Objective
Final step in removing OrganizationUser.AccessAll column:
WARNING: this includes an index rebuild on the
OrganizationUsers
table, because the index previously included theAccessAll
column. I have dropped & recreated it without this column on the assumption it's still useful for the remaining columns in the index. However we may need to coordinate this index rebuild with BRE.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes