)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":38562,"name":"Richard Cruise","email":"rcruise@redhat.com","username":"rcruise"},"change_message_id":"9f71dc5dd6f0afe298d36077e39b90866f0a4e5f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"90aa80cb_eceaa3a3","updated":"2026-05-25 14:14:52.000000000","message":"I just realised this is already a backport. That being the case, only the test fixes are needed (or abandon the change since it\u0027s already bordering on unsuitable for a backport","commit_id":"a4f9d8825d06250ebde99a4f9e4056d1861d8991"},{"author":{"_account_id":38562,"name":"Richard Cruise","email":"rcruise@redhat.com","username":"rcruise"},"change_message_id":"ed20a9c0215a92e827e92ad578ed778b611c7249","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b04a5ff2_f47943ad","updated":"2026-05-25 14:13:08.000000000","message":"I ran the change through Claude Sonnet and it found the following issues. I think only the ones marked Critical and High are worth looking at:\n\nIssues Found\nCritical Issues 🔴\n1. 12 Karma Unit Test Failures — L7 Message Capitalization Not Reflected in Spec\n\nFile: octavia_dashboard/static/app/core/openstack-service-api/lbaasv2.service.spec.js:97,106,113,121,128,135,142,149,383,391,399,407\nIssue: The service code changes error messages for all L7 operations from lowercase \"l7\" to uppercase \"L7\" (e.g., \u0027Unable to retrieve l7 policies.\u0027 → \u0027Unable to retrieve L7 policies.\u0027). However, the test data array in lbaasv2.service.spec.js was not updated accordingly. All 12 L7-related test cases fail with:\n\nExpected spy add to have been called with:\n  [ \u0027error\u0027, \u0027Unable to delete l7 policy.\u0027 ]\nbut actual calls were:\n  [ \u0027error\u0027, \u0027Unable to delete L7 policy.\u0027 ]\n\nImpact: This is a blocking issue. The CI Karma gate will fail. The change cannot be merged in its current state.\nFix: Update all error: entries in the test data array for L7 operations to use uppercase \"L7\". Example:\n\n// Before:\nerror: \u0027Unable to retrieve l7 policies.\u0027,\n// After:\nerror: \u0027Unable to retrieve L7 policies.\u0027,\n\nThe following entries need updating: getL7Policies (×2), getL7Policy (×2), deleteL7Policy, getL7Rules, getL7Rule, deleteL7Rule, createL7Policy, editL7Policy, createL7Rule, editL7Rule.\n\n\n\n2. error.data Is Not Validated as a String — JSON Objects Will Display as [object Object]\n\nFile: octavia_dashboard/static/app/core/openstack-service-api/lbaasv2-error.service.js:29-33\n\nIssue: The implementation does:\n\nif (error \u0026\u0026 error.data) {\n  errorDetail \u003d error.data;\n}\nif (errorDetail) {\n  toastService.add(\u0027error\u0027, message + \u0027 \u0027 + errorDetail);\n}\n\nWhen Angular\u0027s $http service receives a JSON error response (e.g., with Content-Type: application/json), it automatically parses error.data into a JavaScript object. Concatenating an object with a string produces \"[object Object]\", which is less useful than the generic message alone.\n\nThe Octavia API commonly returns structured errors like:\n\n{\"faultstring\": \"Invalid input for field/attribute allowed_cidrs.\"}\n\nor:\n\n{\"NeutronError\": {\"message\": \"...\", \"type\": \"...\"}}\n\nIn these cases, error.data is a JavaScript object, and the concatenation will produce unhelpful output like \"Unable to create listener. [object Object]\".\n\nImpact: The feature as implemented will often not work correctly in production. Many Octavia API errors return structured JSON responses. Users may see [object Object] in toast notifications, which is arguably worse than no detail at all.\n\nSuggestion: The handleError function should check whether error.data is a string or an object, and extract the message appropriately. Example:\n\nfunction handleError(message, error) {\n  var errorDetail \u003d \u0027\u0027;\n  if (error \u0026\u0026 error.data) {\n    if (typeof error.data \u003d\u003d\u003d \u0027string\u0027) {\n      errorDetail \u003d error.data;\n    } else if (error.data.faultstring) {\n      errorDetail \u003d error.data.faultstring;\n    } else if (error.data.message) {\n      errorDetail \u003d error.data.message;\n    }\n  }\n  if (errorDetail) {\n    toastService.add(\u0027error\u0027, message + \u0027 \u0027 + errorDetail);\n  } else {\n    toastService.add(\u0027error\u0027, message);\n  }\n}\n\nThe exact keys to check (faultstring, message, etc.) depend on what the Octavia API and the Python API middleware layer return. The author should verify the actual error response structure. An alternative is to use JSON.stringify(error.data) as a fallback for object types, though a targeted key extraction is more user-friendly.\n\n\nMajor Issues 🟡\n3. Redundant handleError Wrapper Functions in Both Service Files\n\nFile: octavia_dashboard/static/app/core/openstack-service-api/lbaasv2.service.js:73-75 and octavia-barbican.service.js:629-631\nIssue: Both lbaasv2.service.js and octavia-barbican.service.js define a local private handleError function that does nothing but delegate to lbaasv2ErrorService.handleError:\n\nfunction handleError(message, error) {\n  lbaasv2ErrorService.handleError(message, error);\n}\n\nThis indirection provides no value. The .catch() callbacks could call lbaasv2ErrorService.handleError(...) directly, or the factory reference could simply be renamed locally (e.g., var errorHandler \u003d lbaasv2ErrorService;).\n\nImpact: Adds unnecessary code complexity and an extra stack frame in error paths, though in practice this has no functional impact.\nSuggestion: Remove the wrapper and call lbaasv2ErrorService.handleError(message, error) directly in each .catch() callback. If it feels verbose, use a shorter local alias:\n\nvar onError \u003d lbaasv2ErrorService.handleError.bind(lbaasv2ErrorService);\n// then in each catch:\n.catch(function(error) { onError(gettext(\u0027Unable to create listener.\u0027), error); });\n\n4. Service Name lbaasv2-error Does Not Reflect Its Broader Use\n\nFile: octavia_dashboard/static/app/core/openstack-service-api/lbaasv2-error.service.js\nIssue: The new Angular factory is named horizon.app.core.openstack-service-api.lbaasv2-error, suggesting it is specific to the LBaaS v2 API. However, it is also injected into and used by octavia-barbican.service.js, which handles Barbican (certificate/secret) operations. This naming mismatch could confuse future maintainers.\nSuggestion: Consider naming it horizon.app.core.openstack-service-api.octavia-error and renaming the file to octavia-error.service.js to better reflect its shared scope.\n\n\nMinor Issues / Suggestions 🔵\n5. Insufficient New Test Coverage\n\nFile: lbaasv2.service.spec.js and octavia-barbican.service.spec.js\nIssue: The new test cases test only a single happy path (API returns a plain string in error.data). Missing test scenarios:\nCase where error is null (network failure)\nCase where error.data is null/undefined (non-API error)\nCase where error.data is a JavaScript object (JSON response)\nTests for the error service itself (lbaasv2-error.service.js) as a standalone spec file\nSuggestion: Add a dedicated lbaasv2-error.service.spec.js that tests the factory in isolation. Add test cases for all edge cases.\n\n6. Missing Member createMember Error Coverage\n\nFile: octavia_dashboard/static/app/core/openstack-service-api/lbaasv2.service.js\nIssue: Scanning the diff, all API functions were updated, but the createMember function was not visible in the diff. Let the author confirm this function also received the update.\n\n7. Unrelated package-lock.json Upgrade Included in This Change\n\nFile: package-lock.json\nIssue: The package-lock.json is upgraded from lockfileVersion: 1 to lockfileVersion: 3 (npm v7+ format), representing a large diff (the bulk of the 181 additions and 100 removals in --stat). This is an unrelated change that should be in its own commit or at minimum explained in the commit message.\nImpact: This makes the diff very large and harder to review. It also changes the npm lockfile format in a way that may affect other contributors using older npm versions.\nSuggestion: Either separate the package-lock.json update into its own commit, or mention it explicitly in the commit message.\n\n\nNits 🟢\n8. Copyright Year 2026 in New File\n\nFile: octavia_dashboard/static/app/core/openstack-service-api/lbaasv2-error.service.js:8\nComment: Copyright 2026 Red Hat Inc. — this is technically correct given the current date (May 2026) but is unusual to see in practice. No change needed; just noting it.\n\n9. Error Message Concatenation Uses a Space Instead of a Newline\n\nFile: lbaasv2-error.service.js:33\nComment: toastService.add(\u0027error\u0027, message + \u0027 \u0027 + errorDetail) — consider whether a separator other than a plain space would be clearer in the toast notification, e.g., message + \u0027\\n\u0027 + errorDetail or message + \u0027 (\u0027 + errorDetail + \u0027)\u0027. This is a UI preference decision.","commit_id":"a4f9d8825d06250ebde99a4f9e4056d1861d8991"}]}