Show propagated roles on the tag page with a Tag Source column - #602
Draft
barborico wants to merge 3 commits into
Draft
Show propagated roles on the tag page with a Tag Source column#602barborico wants to merge 3 commits into
barborico wants to merge 3 commits into
Conversation
Row-per-entity with a Tag Source column, so a role appearing via two groups is one row with two sources.
Only the top-level propagate_to_roles gate had test coverage; none of the five active-record filters in get_tag's propagation query (OktaGroupTagMap active, RoleGroupMap active, source group not deleted, source group managed, role not deleted) did, so a refactor could silently drop any one of them. Each new test breaks exactly one filter's condition and asserts the row is excluded; verified each is load-bearing by removing its clause and confirming the corresponding test fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A disabled tag propagates nothing, and an unmanaged role enforces nothing, so neither belongs in propagated_to_groups -- listing them would claim reach the enforcement path does not have. Also names the row-merge test for what it asserts and describes the module by what it builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
seems nice! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Shows roles that a tag reaches by propagation on the tag page, and reworks the "Groups with Tag" table from row-per-tag-map to row-per-entity with a Tag Source column.
Stacked on #601 — review and merge that one first. This PR's diff against
mainwill look larger than it is until #601 lands; read it againstbrynna/tag_constraint_propagation.Urgency: TODO — set desired turnaround
Expected review effort: MEDIUM — 3 commits, one non-obvious SQL detail
Motivation
#601 makes a tag's constraints reach roles associated with a tagged group. But the tag page still lists only groups the tag is stored on, so an operator opening the SOX tag cannot see which roles it actually governs — the propagated ones are invisible.
The table also had a pre-existing shortcoming this fixes: a group tagged both directly and via its app produced two rows for the same group.
Description & Screenshots of Changes
Backend.
TagDetailgainspropagated_to_groups, populated by a query inget_tagthat finds roles associated with any group carrying the tag. Each entry names the role, the source group, and whether the association was membership or ownership.The query is gated on
propagate_to_rolesandenabled, and filters five active-record conditions: both theOktaGroupTagMapand theRoleGroupMapmust be active, the source group must be non-deleted and managed, and the role must be non-deleted and managed. A disabled tag propagates nothing and an unmanaged role enforces nothing, so listing either would claim reach the enforcement path does not have.Frontend.
groupsWithTagRows.tsmergesactive_group_tags(direct and app-inherited) withpropagated_to_groupsinto one row per entity, each carrying every source that applies. The column becomes Tag Source with one chip per source —Direct,App: {name},Group: {name}. The remove button shows only on a row with aDirectchip and removes only the direct assignment; app-inherited and propagated rows are removed by removing the app tag or ending the association.Validation of Changes
make testgreen: 924 backend / 63 frontend at head..where(...)clause and confirming only that test fails.enabledgate and the roleis_managedfilter were verified the same way.Guidance for Reviewers
Read commit-by-commit; three commits, each green.
One thing worth a careful look: the
aliased()inget_tag's query.RoleGroupis joined-table inheritance onokta_group, so selecting both it and the plain sourceOktaGroupmakes the two overlap on one physical table. Left unaliased, SQLAlchemy silently auto-aliasesRoleGroup's implicit parent join, the laterwhereclauses bind to that alias instead of the source-group row, and the query returns zero rows with no error. That bug was live during development and is exactly the kind a passing test suite would not have caught without the negative filters.The
Tag Sourcecolumn also changes the table's "Total" from a tag-map count to a row count — correct for a row-per-entity table, but operators eyeball that number, so it belongs in release notes.🤖 Generated with Claude Code