-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report: group third-party entities #14655
Merged
Merged
Changes from 43 commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
a9439c1
squashed: group tables by entities
alexnj 2f3076b
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj ce00b64
Implement third-party-summary handling without breaking backward compat.
alexnj 6047d39
bugfix: Unattributable rows were getting removed.
alexnj 87a7d83
would would?
alexnj 46a9e1e
lint error.
alexnj f4287fc
Review comments from #14697
alexnj fd5b410
Implement report-side post-aggregation sorting
alexnj ca8674d
more formal support for pre-aggregated table/opportunity
alexnj 0f51523
i18n changes.
alexnj 32ed520
Strings
alexnj fad616f
Bugfix: Exclude non-entity-marked results from aggregation.
alexnj ccdb08c
string proto updates
alexnj b41b197
Distribute sort order to audits
alexnj 57ed639
Deprecate third-party-summary LinkValue
alexnj cba2bdd
Review changes: shorter sortedBy
alexnj b340de9
reinstate LinkValue. We're not there yet.
alexnj 3e808ba
revert unintended change
alexnj 993512b
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 425d1c0
Adjust for backCompat refactor
alexnj 183c65d
cleanup indentation
alexnj ed9587a
revert changes to fixture
alexnj b7c9ec2
Fixup existing tests.
alexnj 1b8c824
Add a test to ensure compatibility upgrade works
alexnj 105689d
comments
alexnj a2a2d5d
Validate design guidelines: no zebra for aggregation
alexnj 5774426
Apply suggestions from code review
alexnj 1a59745
review changes
alexnj 2762cc8
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj b0665fd
adjust to new entity classification format
alexnj 08950a1
Apply suggestions from code review
alexnj 00ff7f4
Review changes.
alexnj d07bdd0
skipSumming refactor
alexnj cfa8493
move skipGrouping to makeTable/OpportunityDetails
alexnj 6fcfb3e
rename skipGrouping to isEntityGrouped
alexnj 7de44f8
update comment to reflect new naming
alexnj 6a4cd56
adding tests for table grouping
alexnj 8b8daee
Apply suggestions from code review
alexnj 188d657
Review changes
alexnj b2148aa
revert the relaxed typechecks for arithmetic
alexnj ebd6e96
cosmetic: fix indentation of legacy lhr + new viewer
alexnj 6c2b2ac
bugfix: verify zebra-style on third-party checkbox change
alexnj 583ec96
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 2b22e4c
Add a console warning for unknown types + tests for that
alexnj 0a1bc1f
review changes.
alexnj a27d053
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 7b50d93
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 3990971
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 312724b
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 3ac1b06
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj f708a49
Update header styles to latest design + hover state.
alexnj 90eff93
Update color codes for table from UX.
alexnj eb7b976
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 9c3b145
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj 6799be9
Merge remote-tracking branch 'origin/main' into entity-based-3p-grouping
alexnj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
Having this many optional params would work a lot better as an options object.
That being said, it's not worth making that change in this PR since it would affect a multitude of audits so this is fine for now.
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.
Yeah I had the same feeling as I was expanding the list. We should make it an object at some point. I can follow up as a later refactor PR.
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.
This seems worth working out first.
If we consider
Audit.makeTableDetails
part of our stable API (we probably should), then to defer this change post-10.0 we will need to support both all the params one-by-one, plus a single options object (or maybeheadings, results, opts
for just the optionals, but at that point should probably just collapse all of them into the object). Checking at runtime the number of parameters (or whatever) wouldn't be a big deal, and actually doing a breaking change here will just needlessly break custom audits, so I can see us doing that indefinitely.Whatever approach we do, we certainly don't want to release a version that has 2 params + 4 optional params, making us have to then support 3 interfaces here indefinitely. So I'd consider this important to address before landing this PR (though, the work to make this a options object should certainly be its own PR)
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.
What should be the signature of the refactored function —
makeTableDetails(heading, results, options)
, with summary collapsed into the object? Who is depending on this other than LH/audit code?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.
Custom audits.
That, or just one options param for everything. Up for debate. Perfect for a (small) PR!
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.
#14753
Once finalized, I'll adjust this PR accordingly - should be trivial.