-
Notifications
You must be signed in to change notification settings - Fork 1
Adds async identity collection to the audit UI #191
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
base: main
Are you sure you want to change the base?
Adds async identity collection to the audit UI #191
Conversation
jwrosewell
commented
Jun 15, 2022
- Controller for the audit UI component now supports the AuditHandler interface.
- Audit viewer is now added to the advert div element via the AuditHandler interface in the demo. Note the demo has been amended to "fake" an auditable response to demonstrate the UI. More work is needed on system integration to align the different components of the project. See issue Prebid "bidWon" event does not contain PAF object #181.
- Identity information when provided adds an email icon with a pre-populated email and a link to the privacy policy.
- All class for identity data are performed asynchronously.
- Common elements of the CMP concerning localisation have been moved to the core.
- This PR along with Adding initial version of audit log v2 html pattern library #180 are dependencies for the update to the V2 UI.
…d with the transmission. Enabled the contact DPO, and privacy policy URL icons when identity information is returned. Handled the situation where identity information can not be returned. Modified the data model and fields to support promise resolution to update the UI. Moved the tick and cross icons into the component HTML.
…t-log-async # Conflicts: # paf-mvp-audit/src/controller.ts
…be shared across the CMP and the Audit modules.
…e rather than a folder. Uses the shared core loader and locale features.
…at the audit handler can extract them if the users wants to check the log.
…ansfer the audit log data.
… into feature/audit-log-async
OlivierChirouze
left a comment
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.
I'm fine with this PR, but would like to have @RomainLofaso to check the integration with seeds paf-lib.ts
paf-mvp-audit/src/model.ts
Outdated
| * and the other identity information over multiple requests to the same endpoint. | ||
| * @returns promise that when resolved will provide the identity associated with the transmission result or an error. | ||
| */ | ||
| public getIdentity() { |
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.
NIT: return type
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.
Thanks. There are quite a few changes coming in the next PR. I'm considering changing this one to draft and then adding the next set of changes to it. Could @RomainLofaso hold off reviewing until we've updated?
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.
Would you mind keeping it separate instead?
This way it would be easier to review: in the current review I think there are only a couple of files that "need" to be reviewed by Romain, the rest is mostly about translation, identity and pattern library.
I'm wondering if adding "quite a few changes" to this review would not make it complicated to review.
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.
The async model had to change a lot when the V2 UI layer was finally complete and the two combined. I'm reviewing this today. Therefore some of the elements of this PR are no longer present and it would merely waste review time. I don't think there's a way to get away from the additional features of the V2 layer being a large addition (not change) to the project. Perhaps the review could include a virtual F2F to go through the approach if the concern is about the size of the PR?
… into feature/audit-log-v2-complete # Conflicts: # paf-mvp-demo-express/public/assets/.gitignore # paf-mvp-demo-express/src/views/publisher/index.hbs # tsconfig.test.json
…f-mvp-implementation into feature/audit-log-v2-complete
…to fix a bug in the choosen project.
… the audit viewer to support build of crypto components for browser.
…t come from OpenRTB to be able to demonstrate the mock audit log.
|
There were a number of issues related to the integration of the V2 changes which make this PR unavoidably large. I suggest the following approach to review.
The other commits after these are minor and relate to fixing missing NPM packages or the demo code. Note that the workflow tests needed to change to include getting the submodule as a dependency. At the time of writing this these tests are queued but not complete. |
Added sub module clone to build-test script after confirming worked resolve missing module on cypress.
Removed async/await syntax from jest test as might be the cause of intermittent failure of audit tests that require promises. Updated the audit log mocks from the refreshed tests.
|
@OlivierChirouze This is now ready for review. I know it is a large PR but the main areas to review are the core changes related to making crypto async. See this commit. |
The example data is now copied before the mock audit log is created to avoid the order of test execution impacting the test outcomes. (cherry picked from commit ada897d4b80bcb1aa8e2ffde4c69bf20b3343c91)
…n a form that will fail validation. This commit resigns the data until it passes validation.


