Enhance Brand Form UI/UX and Fix Workfront Data Persistence#12
Enhance Brand Form UI/UX and Fix Workfront Data Persistence#12davidbenge wants to merge 6 commits intomainfrom
Conversation
- Add eventSanitizer utility to redact sensitive data from logs - Sanitizes secrets, tokens, passwords, API keys, and auth headers - Redacts query parameters from URLs - Deep sanitizes nested objects with configurable recursion depth - Update all event handlers to log incoming events securely: - agency-event-handler - agency-registration-internal-handler - agency-assetsync-internal-handler - adobe-product-event-handler - Enhance agency registration testing: - Add comprehensive test for registration.enabled event with demo data - Verify Agency object values (agencyId, brandId, name) - Verify agencyEndPointUrl built from app_runtime_info - Test complete registration flow from agency-event-handler to internal handler - Add registration-enabled event fixture from agency - Improve Agency and AgencyManager classes: - Fix agency name and endpoint URL handling - Enhance agency identification and validation - Update UI components (AgencyListView, AgencyRegistrationList) - Update event handling architecture: - Enhance AppEventRegistry and ProductEventRegistry - Improve event routing and validation - Add event handler routing documentation - Reorganize event documentation: - Add AEM event samples with sync variations - Add agency-to-brand registration event samples - Update event README with better examples This commit ensures all event logs are secure by default and adds comprehensive testing for agency registration flows.
- Update agency-event-handler tests to expect fully qualified action names - Change 'agency-registration-internal-handler' to 'a2b-brand/agency-registration-internal-handler' - Change 'agency-assetsync-internal-handler' to 'a2b-brand/agency-assetsync-internal-handler' - Fix error message assertion for unhandled event types - Update from 'Unhandled event type' to 'Event definition not found' All tests now pass successfully (60 passed, 60 total)
- Fix /registrations route to use AgencyRegistrationList instead of AgencyListView - Update AgencyRegistrationList to use correct viewProps structure (aioRuntimeNamespace, imsToken, imsOrg) - Fix API service initialization to construct base URL from runtime namespace - Update WorkfrontConfigModal props to use correct viewProps fields - Add Workfront integration UI components and services - Implement Workfront event registry and subscription management This ensures the Workfront configuration modal is visible and functional on the agency registrations view page.
- Refactor Agency form layout and user experience - Change layout from maxWidth size-5000 to size-6000 with better padding - Improve screen space utilization without over-stretching - Enhance overall form aesthetics and usability - Enhance Workfront integration section - Wrap all Workfront fields in Flex column layout for vertical stacking - Add consistent size-200 gap between URL, Company, and Group fields - Set width 100% on all fields for proper container spanning - Auto-load companies/groups when opening edit form with existing data - Add dynamic required indicators (Company/Group required when URL provided) - Implement validation error display with auto-clear on selection - Clear validation errors when Workfront URL is removed - Improve data loading and persistence - Load Workfront companies and groups on form initialization in edit mode - Properly populate dropdowns with current selections - Fix issue where dropdowns were empty/unselectable with existing data - Add comprehensive validation and error handling - Display inline validation errors on Picker components - Auto-clear errors when user makes selections - Clear all Workfront errors when URL field is cleared - Provide immediate feedback on validation state - Add documentation - Create AGENCY_FORM_REFACTOR.md with detailed implementation notes - Document all changes, benefits, and testing checklist - Include before/after comparisons and feature summaries This refactor brings the Agency form in line with the Brand form improvements made in a2b-agency, ensuring a consistent user experience across both applications.
davidbenge
requested review from
Mazhinyan,
adobesam,
atuljadhavadobe,
Copilot and
gallardo-adobe
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| agency = await agencyManager.updateAgency(agencyId, { | ||
| enabled: false, | ||
| disabledAt: eventData.disabledAt ? new Date(eventData.disabledAt) : new Date(), | ||
| name: eventData.name || agency.name, | ||
| endPointUrl: agencyEndpointUrl, | ||
| agencyEndPointUrl: agencyEndpointUrl, | ||
| agencyName: agencyName || agency.agencyName | ||
| }); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix this problem, remove the assignment to agency while ensuring that the asynchronous function is still called and awaited for side effects. Change
agency = await agencyManager.updateAgency(agencyId, { ... });to
await agencyManager.updateAgency(agencyId, { ... });This preserves the side effects of the function call and ensures that code behavior is unchanged. Only the assignment is removed.
Edit only the relevant line in src/actions/event-handlers/agency-registration-internal-handler/index.ts within the shown snippet.
| @@ -302,7 +302,7 @@ | ||
| logger.info(`Disabling agency ${agencyId}`); | ||
| // Update existing agency with disabled status | ||
| // Keep the secret so it can be re-enabled later if needed | ||
| agency = await agencyManager.updateAgency(agencyId, { | ||
| await agencyManager.updateAgency(agencyId, { | ||
| enabled: false, | ||
| disabledAt: eventData.disabledAt ? new Date(eventData.disabledAt) : new Date(), | ||
| name: eventData.name || agency.name, |
| } | ||
|
|
||
| // Parse runtime info | ||
| const runtimeInfo = new ApplicationRuntimeInfo( |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
The best way to fix this issue is to delete the initialization of the unused variable runtimeInfo, including the assignment and the line break it occupies (lines 62–64). This resolves the CodeQL warning and slightly improves readability and maintainability (removes dead code). Since no other code references runtimeInfo, no other modifications are necessary. No changes to imports, method signatures, or logic elsewhere in the file are needed.
| @@ -59,9 +59,6 @@ | ||
| } | ||
|
|
||
| // Parse runtime info | ||
| const runtimeInfo = new ApplicationRuntimeInfo( | ||
| JSON.parse(params.APPLICATION_RUNTIME_INFO) | ||
| ); | ||
|
|
||
| // Initialize Agency Manager | ||
| const agencyManager = new AgencyManager(params.LOG_LEVEL || 'info'); |
| } | ||
|
|
||
| // Parse runtime info | ||
| const runtimeInfo = new ApplicationRuntimeInfo( |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, the unused variable runtimeInfo should be removed from the code, unless its instantiation is necessary to trigger essential side effects (which, without supporting evidence, we assume is not the case).
This means deleting or commenting out the following lines:
80: const runtimeInfo = new ApplicationRuntimeInfo(
81: JSON.parse(params.APPLICATION_RUNTIME_INFO)
82: );No further changes are needed if there is no requirement for this variable elsewhere in the shown code.
| @@ -77,9 +77,6 @@ | ||
| } | ||
|
|
||
| // Parse runtime info | ||
| const runtimeInfo = new ApplicationRuntimeInfo( | ||
| JSON.parse(params.APPLICATION_RUNTIME_INFO) | ||
| ); | ||
|
|
||
| // Initialize Agency Manager | ||
| const agencyManager = new AgencyManager(params.LOG_LEVEL || 'info'); |
| import { | ||
| View, | ||
| Form, | ||
| TextField, | ||
| Button, | ||
| Flex, | ||
| Heading, | ||
| Text, | ||
| Switch, | ||
| StatusLight, | ||
| Divider, | ||
| Content, | ||
| Header, | ||
| Image, | ||
| Well, | ||
| Picker, | ||
| Item, | ||
| ProgressCircle | ||
| } from '@adobe/react-spectrum'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
The best way to fix unused imports is to remove them entirely from the import statement. In this case, Divider and Image are imported from @adobe/react-spectrum but never used. The import statement on lines 4–22 should be edited to remove these two names but leave the others untouched. No additional changes or imports are required elsewhere in the code, and this edit will not affect functionality since they are not being used.
| @@ -11,10 +11,8 @@ | ||
| Text, | ||
| Switch, | ||
| StatusLight, | ||
| Divider, | ||
| Content, | ||
| Header, | ||
| Image, | ||
| Well, | ||
| Picker, | ||
| Item, |
There was a problem hiding this comment.
Pull Request Overview
This PR implements Workfront integration for the a2b-brand application, adding the ability to configure Workfront settings, manage event subscriptions, and handle Workfront events. It also fixes several bugs related to agency registration and adds comprehensive event sanitization for logging.
Key Changes
- Added Workfront integration with company/group configuration UI and API endpoints
- Implemented WorkfrontClient for API interactions with S2S token caching (21-hour TTL)
- Added event sanitization utilities to prevent sensitive data exposure in logs
- Fixed agency name/endpoint URL handling in registration events
- Enhanced registration.disabled event support with comprehensive testing
Reviewed Changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/actions/services/workfront/*.ts | New Workfront service actions for listing companies/groups, configuration, and subscription management |
| src/actions/services/workfront/WorkfrontClient.ts | Workfront API client with S2S authentication and cached token support |
| src/actions/utils/tokenCache.ts | Token caching utility for S2S tokens with 21-hour TTL in state store |
| src/actions/utils/eventSanitizer.ts | Event sanitization for secure logging, redacting secrets and tokens |
| src/actions/classes/WorkfrontEventRegistry.ts | Registry of Workfront event definitions (projects, tasks, documents, etc.) |
| src/actions/classes/AppEventRegistry.ts | Updated app event definitions with handler action names and proper field requirements |
| src/actions/event-handlers/agency-registration-internal-handler/index.ts | Enhanced to handle registration.disabled events and fix agency name/endpoint URL bugs |
| src/dx-excshell-1/web-src/src/components/layout/AgencyForm.tsx | New agency form component with Workfront configuration UI |
| src/actions/classes/Agency.ts | Added Workfront fields and disabledAt timestamp |
| app.config.yaml | Added new Workfront service actions with proper runtime configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| statusCode: 400, | ||
| body: { | ||
| error: 'Invalid S2S_SCOPES format', | ||
| message: 'S2S_SCOPES must be a valid JSON array string (e.g., \'["AdobeID","openid"]\')', |
There was a problem hiding this comment.
The error message uses escaped single quotes within single quotes which may be confusing. Consider using template literals or adjusting the quoting for better readability.
| message: 'S2S_SCOPES must be a valid JSON array string (e.g., \'["AdobeID","openid"]\')', | |
| message: `S2S_SCOPES must be a valid JSON array string (e.g., ["AdobeID","openid"])`, |
| statusCode: 400, | ||
| body: { | ||
| error: 'Invalid S2S_SCOPES format', | ||
| message: 'S2S_SCOPES must be a valid JSON array string (e.g., \'["AdobeID","openid"]\')', |
There was a problem hiding this comment.
The error message uses escaped single quotes within single quotes which may be confusing. Consider using template literals or adjusting the quoting for better readability.
| message: 'S2S_SCOPES must be a valid JSON array string (e.g., \'["AdobeID","openid"]\')', | |
| message: `S2S_SCOPES must be a valid JSON array string (e.g., '["AdobeID","openid"]')`, |
| } | ||
|
|
||
| // Update brand with subscription IDs | ||
| (agency as any).workfrontEventSubscriptions = subscriptionIds; |
There was a problem hiding this comment.
Using 'as any' bypasses TypeScript's type safety. Consider updating the IAgency interface to include workfrontEventSubscriptions or create a typed update method in AgencyManager.
| } | ||
|
|
||
| // Clear subscription IDs from brand | ||
| (agency as any).workfrontEventSubscriptions = []; |
There was a problem hiding this comment.
Using 'as any' bypasses TypeScript's type safety. Consider updating the IAgency interface to include workfrontEventSubscriptions or create a typed update method in AgencyManager.
| // Update agency with Workfront configuration | ||
| // Update the agency object properties | ||
| (agency as any).workfrontServerUrl = params.workfrontServerUrl; | ||
| (agency as any).workfrontCompanyId = params.workfrontCompanyId; | ||
| (agency as any).workfrontCompanyName = params.workfrontCompanyName; | ||
| (agency as any).workfrontGroupId = params.workfrontGroupId; | ||
| (agency as any).workfrontGroupName = params.workfrontGroupName; | ||
| (agency as any).workfrontEventSubscriptions = []; // Will be populated when subscriptions are created | ||
|
|
||
| // Save the updated agency | ||
| const updatedAgency = await agencyManager.saveAgency(agency); |
There was a problem hiding this comment.
Multiple uses of 'as any' to bypass TypeScript type safety. Since the Agency class is readonly, consider using AgencyManager's updateAgency method instead of direct property assignment, which would maintain type safety.
| // Update agency with Workfront configuration | |
| // Update the agency object properties | |
| (agency as any).workfrontServerUrl = params.workfrontServerUrl; | |
| (agency as any).workfrontCompanyId = params.workfrontCompanyId; | |
| (agency as any).workfrontCompanyName = params.workfrontCompanyName; | |
| (agency as any).workfrontGroupId = params.workfrontGroupId; | |
| (agency as any).workfrontGroupName = params.workfrontGroupName; | |
| (agency as any).workfrontEventSubscriptions = []; // Will be populated when subscriptions are created | |
| // Save the updated agency | |
| const updatedAgency = await agencyManager.saveAgency(agency); | |
| // Update agency with Workfront configuration using AgencyManager's updateAgency method | |
| const updateFields = { | |
| workfrontServerUrl: params.workfrontServerUrl, | |
| workfrontCompanyId: params.workfrontCompanyId, | |
| workfrontCompanyName: params.workfrontCompanyName, | |
| workfrontGroupId: params.workfrontGroupId, | |
| workfrontGroupName: params.workfrontGroupName, | |
| workfrontEventSubscriptions: [] // Will be populated when subscriptions are created | |
| }; | |
| const updatedAgency = await agencyManager.updateAgency(params.agencyId, updateFields); |
| adobe-product-event-handler: | ||
| function: src/actions/event-handlers/product/adobe-product-event-handler/index.ts | ||
| web: 'yes' | ||
| web: 'no' |
There was a problem hiding this comment.
The adobe-product-event-handler was changed from web: 'yes' to web: 'no' and require-adobe-auth from true to false. This is a potentially breaking change that could affect how Workfront and other Adobe products send events to this handler. Ensure this aligns with the intended architecture and that Workfront events can still reach this handler.
| const runtimeInfo = new ApplicationRuntimeInfo( | ||
| JSON.parse(params.APPLICATION_RUNTIME_INFO) | ||
| ); |
There was a problem hiding this comment.
Unused variable runtimeInfo.
| const runtimeInfo = new ApplicationRuntimeInfo( | |
| JSON.parse(params.APPLICATION_RUNTIME_INFO) | |
| ); |
| } | ||
|
|
||
| // Parse runtime info | ||
| const runtimeInfo = new ApplicationRuntimeInfo( |
There was a problem hiding this comment.
Unused variable runtimeInfo.
| import { | ||
| View, | ||
| Form, | ||
| TextField, | ||
| Button, | ||
| Flex, | ||
| Heading, | ||
| Text, | ||
| Switch, | ||
| StatusLight, | ||
| Divider, | ||
| Content, | ||
| Header, | ||
| Image, | ||
| Well, | ||
| Picker, | ||
| Item, | ||
| ProgressCircle | ||
| } from '@adobe/react-spectrum'; |
There was a problem hiding this comment.
Unused imports Divider, Image.
| agency = await agencyManager.updateAgency(agencyId, { | ||
| enabled: false, | ||
| disabledAt: eventData.disabledAt ? new Date(eventData.disabledAt) : new Date(), | ||
| name: eventData.name || agency.name, | ||
| endPointUrl: agencyEndpointUrl, | ||
| agencyEndPointUrl: agencyEndpointUrl, | ||
| agencyName: agencyName || agency.agencyName | ||
| }); |
There was a problem hiding this comment.
The value assigned to agency here is unused.
| this.logger.debug('Listing Workfront companies'); | ||
| const headers = await this.getAuthHeaders(); | ||
|
|
||
| const response = await this.axiosClient.get('/attask/api/v15.0/company/search', { |
There was a problem hiding this comment.
The latest API version is 21.
It might be a good idea to introduce version control (e.g., define a prefixed constant) so the API version can be managed from one central place.
Also, if I’m not mistaken, when no version is specified, the request defaults to the latest version of the Workfront API.
There was a problem hiding this comment.
lets do that in the next pull. i will patch this up to 21 and let you then have a go at it and fix up and add in versioning. sound ok?
| const headers = await this.getAuthHeaders(); | ||
|
|
||
| const response = await this.axiosClient.post( | ||
| '/attask/api/v15.0/eventsub', |
There was a problem hiding this comment.
@davidbenge I am not shur but If I am not mistaken we do not have this api.
for getting subscriptions we have
GET /attask/eventsubscription/api/v1/subscriptions
POST /attask/eventsubscription/api/v1/subscriptions
DELETE /attask/eventsubscription/api/v1/subscriptions/{id}
There was a problem hiding this comment.
your right let me update that
davidbenge
dismissed
adobesam’s stale review
The merge-base changed after approval.
Enhance Brand Form UI/UX and Fix Workfront Data Persistence
Overview
This PR significantly improves the Brand management form with enhanced UI/UX, fixes critical Workfront data persistence issues, and implements comprehensive validation for Workfront integration fields.
Problem Statement
Data Persistence Issues
UI/UX Issues
Solution
🔧 Data Persistence Fixes
Root Cause:
DemoBrandManager.getBrandFromJson()andcreateBrand()weren't mapping Workfront fields when creating Brand instances from API responses.Fix:
🎨 UI/UX Improvements
Layout Enhancement
size-5000tosize-6000for better space utilizationgray-50background aligned with header bar (no gaps)size-200tosize-400for better spacingWorkfront Section Refactor
Flex direction="column"size-200gap between fieldswidth="100%"on all fields for proper container spanningValidation Enhancement
Visual Improvements
Files Changed
Core Logic
src/dx-excshell-1/web-src/src/utils/DemoBrandManager.ts- Added Workfront field mappingsrc/dx-excshell-1/web-src/src/components/layout/BrandForm.tsx- Complete UI/UX refactorsrc/dx-excshell-1/web-src/src/components/layout/BrandManagerView.tsx- Updated state managementBackend
src/actions/classes/BrandManager.ts- Enhanced brand managementsrc/actions/services/workfront/WorkfrontClient.ts- Improved Workfront API clientsrc/actions/services/workfront/list-workfront-companies/index.ts- Updated company listingsrc/actions/services/workfront/list-workfront-groups/index.ts- Updated group listingDocumentation
docs/cursor/WORKFRONT_DATA_PERSISTENCE_FIX.md- Comprehensive fix documentationdocs/apis/workfront/groups/groups_response.json- API response examplesCleanup
src/dx-excshell-1/web-src/src/components/modals/WorkfrontConfigModal.tsx- Replaced with inline formTesting Performed
✅ Data Persistence
✅ Validation
✅ Layout
✅ Workfront Integration
Screenshots
Before
After
Breaking Changes
None - all changes are backward compatible.
Deployment Notes
Related Issues
Fixes issues with:
Checklist
Reviewers
Please verify: