[shaohuzhang1]

feat: add overview export permissions and update related components

[shaohuzhang1]

Overall, the tests cover the intended behavior, but they are somewhat brittle and may pass/fail for the wrong reason.

Key issues and suggestions:

1. Inconsistent permission mocking

   • UserResourcePermissionSerializer is patched in the first test but not the second.
   • If permission validation runs before the duplicate-name check in the second test, the test may hit real permission logic and fail for an unrelated reason.
   • Patch it consistently in both tests, or move the patch into a helper/setUp context.

2. MagicMock provider is too permissive

   • self.provider = MagicMock() will return truthy mocks for any unexpected method call, which can hide bugs.
   • Prefer MagicMock(spec=...) / create_autospec(...) and configure every provider method used by the serializer.
   • At minimum, document which provider contract the test depends on.

3. Permission serializer mock is also too permissive

   • Patching the class without configuring is_valid, validated_data, etc. may silently pass validation.
   • Configure the mocked return values explicitly so the test verifies the intended path.

4. Plaintext password

   • User.objects.create(password="password", ...) stores an unhashed password in tests.
   • Prefer User.objects.create_user(...) or call set_password(...) if using a custom user model.

5. Hardcoded role string

   • role="ADMIN" is a magic string. Use the project’s role enum/constant if available.

6. Workspace IDs may not match the field type

   • "workspace-a" / "workspace-b" are fine only if workspace_id is a text field.
   • If workspace_id is a UUID field, use valid UUID strings.

7. Second test should assert no row was created

   • Add a count assertion after assertRaises:

     self.assertEqual(Model.objects.filter(workspace_id="workspace-a", name=MODEL_NAME).count(), 1)

   • Also assert the exact exception message/code if possible, e.g. assertRaisesRegex(...), so it does not pass because of an unrelated AppApiException.

8. First test should assert full side effects

   • Checking only result["id"] workspace is good, but also assert counts:

     self.assertEqual(Model.objects.filter(workspace_id="workspace-a", name=MODEL_NAME).count(), 1)
     self.assertEqual(Model.objects.filter(workspace_id="workspace-b", name=MODEL_NAME).count(), 1)

9. Narrow the failing operation

   • In the rejection test, construct the serializer outside the assertRaises context if possible, and assert that only .insert(...) raises. This avoids passing because of an unrelated construction/validation error.
   • If construction must happen inside, assert the exception message/code.

10. Minor optimization

    • Since the user is only read in these tests, consider creating it with setUpTestData instead of setUp to reduce repeated DB setup.
    • Keep self.provider in setUp because it is a lightweight mock.

11. Patch target may be brittle

    • patch("models_provider.serializers.model_serializer.ModelProvideConstants", ...) is fine only if the serializer references that exact module attribute. If it imports the constant directly elsewhere, the patch may not apply. Confirm the patch target matches the lookup location.

Recommended shape:

with (
    patch("models_provider.serializers.model_serializer.ModelProvideConstants",
          {"test_provider": self.provider}),
    patch("models_provider.serializers.model_serializer.UserResourcePermissionSerializer") as permission_serializer,
):
    permission_serializer.return_value.is_valid.return_value = True
    permission_serializer.return_value.validated_data = {}
    ...

Bottom line: the test intent is good, but the mocks are too broad, permission patching is inconsistent, and the rejection test should verify that the duplicate-name rule is the actual cause of failure.

[shaohuzhang1]

The patch is syntactically consistent with the existing PermissionConstants enum pattern. I do not see an immediate syntax error, but the following points should be verified:

1. bit_index uniqueness and bit width

   • Confirm bit_index=43 and bit_index=32 are not already used within the same permission group/sub-group.
   • Pay special attention to RESOURCE_APPLICATION_OVERVIEW_EXPORT with bit_index=32. If permission masks are stored as 32-bit signed integers, bit index 32 (and 43) will overflow. Ensure the mask is stored as uint64/bigint or otherwise supports these indexes.

2. Authorization scope / least privilege

   • APPLICATION_OVERVIEW_EXPORT grants export to RoleConstants.USER. Confirm this is intended and that export does not expose sensitive workspace data to all users. If it does, consider restricting to WORKSPACE_MANAGE or a more specific role.
   • Confirm scope=[WORKSPACE_RESOURCE, WORKSPACE] is correct. If export is workspace-level only, including WORKSPACE_RESOURCE may unintentionally allow resource-scoped access.

3. PermissionMeta consistency

   • The first entry does not set is_ee, while the second sets is_ee=is_ee. Confirm this difference is intentional and matches neighboring permissions.
   • Confirm resource_permission_group_list=[ResourcePermissionConst.APPLICATION_MANAGE] is required for the workspace export and that the constant name is correct.
   • Confirm the resource permission does not also need a resource_permission_group_list.

4. Naming and placement

   • The names and placement before the corresponding BACKEND_API_ACCESS entries look consistent with the existing pattern.
   • No obvious optimization is needed; using a helper/factory would only be worthwhile if this enum pattern is repeated heavily.

5. Tests / enforcement

   • Add or verify tests for:
     • unique bit_index values,
     • correct role/scope/category metadata,
     • API enforcement of the new export permissions,
     • EE-only behavior for RESOURCE_APPLICATION_OVERVIEW_EXPORT.
   • If permissions are cached or persisted elsewhere, ensure the new permissions are refreshed/migrated appropriately.

Overall: no definite bug from the diff alone, but the main risks are bit-index collision/overflow and overly broad export authorization for RoleConstants.USER.

[shaohuzhang1]

Review of the diff

1. Correctness / Reactivity

• computed will only update if its dependencies are reactive.

  • props.applicationId is reactive, so that part is fine.
  • isSystemResource() and the perm.application.*.overviewExport() methods must read from reactive state. If perm is a plain module object populated once at startup, canExport will not update when permissions change (e.g. after login, role switch, or async permission loading). Verify perm is backed by a reactive store/composable.
  • Also check what isSystemResource() does. If it internally uses inject() / getCurrentInstance() / useRoute(), calling it inside a computed getter can be unsafe, because composables that rely on inject() must be called synchronously during setup(). Safer pattern:

    const isSystem = computed(() => isSystemResource()) // only if pure/reactive global
    // or, if it is static for this component:
    const isSystem = isSystemResource()

    If it uses inject, resolve it at setup top-level.

• Guard against an empty applicationId.

  if (!props.applicationId) return false

  Otherwise workspace.overviewExport('') may return an unexpected result.

• Verify the permission API shape. perm.application.system.overviewExport() and perm.application.workspace.overviewExport(id) are invoked as functions. If any of them are actually booleans/getters, this will throw at runtime. The inconsistent shape (system vs. workspace) also suggests a unified helper would be cleaner.

2. Security

• Hiding the button with v-if is only a UX control, not a security boundary. Ensure the export API enforces the same permission server-side.
• Consider also guarding the handler for defense in depth:

  function handleExport() {
    if (!canExport.value) return
    // ...
  }

  This protects against stale state or programmatic calls.

3. UX

• v-if removes the button from the DOM, causing layout shift. If the export button is in a fixed toolbar, consider v-show or rendering it disabled with a tooltip explaining missing permission. Either is fine; just be consistent with the rest of the app.
• The :disabled="loading" state does not include exporting, though :loading usually blocks clicks. No issue, just noting.

4. Naming / maintainability

• canExport is very generic in this component. Prefer canExportRanking or canExportOverviewRanking.
• The permission logic is likely needed elsewhere. Consider extracting a composable such as useOverviewExportPermission(applicationId) or a v-permission directive to avoid duplicating isSystemResource() branching.
• Import ordering: perm and isSystemResource are mixed into the import block. If the project enforces import sorting/grouping (ESLint import/order), this may fail lint. Minor.

5. Redundant Boolean(...)

• If the permission functions are typed as boolean, the Boolean(...) wrappers are redundant. Keep them only if they can return undefined/null/truthy non-booleans. Typing the permission methods as boolean would be clearer.

Suggested revision

const isSystem = computed(() => isSystemResource())

const canExportRanking = computed(() => {
  if (!props.applicationId) return false

  if (isSystem.value) {
    return Boolean(perm.application.system.overviewExport())
  }
  return Boolean(perm.application.workspace.overviewExport(props.applicationId))
})

function handleExport() {
  if (!canExportRanking.value) return
  // ...
}

<el-button
  v-if="canExportRanking"
  plain
  :loading="exporting"
  :disabled="loading"
  @click="handleExport"
>

Bottom line

The change is directionally correct, but verify:

1. perm and isSystemResource() are reactive (otherwise canExport will be stale).
2. isSystemResource() does not rely on inject() when called inside computed.
3. The overviewExport permission entries are actually callable functions.
4. The server enforces the export permission independently of the UI.