Merge pull request #1427 from LeanerCloud/fix/qa-admin-user-mgmt

fix(admin): validate group combos, fix checkbox UX, remove stale bulk checkboxes

Author: cristim

.golangci.yml

@@ -133,6 +133,9 @@ linters:
           - misspell
         path: (internal/config/(store_postgres|store_postgres_pgxmock_test|store_postgres_ladder|types|interfaces)|pkg/ladder/(store|types_test)|internal/api/(handler_history|handler_ri_exchange(_test)?|router_handlers_test|coverage_extras_test)|internal/mocks/stores|internal/purchase/scheduled_fire_test)\.go
         text: "cancelled|initialised"
+      - linters:
+          - misspell
+        path: internal/database/postgres/migrations/000082_rename_cancelled_to_canceled_test\.go
     paths:
       - third_party$
       - builtin$

frontend/src/__tests__/users.test.ts

@@ -11,9 +11,10 @@ import {
   selectedUserIds,
   setAllUsers,
   setAvailableGroups,
-  clearSelectedUserIds
+  clearSelectedUserIds,
+  removeSelectedUserId
 } from './state';
-import { showError, showSuccess } from './utils';
+import { showError, showSuccess, validateGroupCombination } from './utils';
 import { confirmDialog } from '../confirmDialog';
 import { applyFilters } from './filters';
 import { renderUsers, renderUserStats, populateBulkGroupSelect } from './userList';
@@ -200,21 +201,50 @@ export async function bulkAddToGroup(groupId: string): Promise<void> {
     return;
   }
 
-  try {
-    // Get current user data and add group
-    const updates = Array.from(selectedUserIds).map(async userId => {
-      const user = allUsers.find(u => u.id === userId);
-      if (!user) return;
+  // Partition the selected users by whether adding this group would produce a
+  // contradictory combination (view-only group + write-capable group, issue
+  // #1405). The bulk path must enforce the same rule as the inline expand
+  // panel and the create/edit modal -- otherwise selecting a Viewer-group
+  // user and bulk-adding a write group silently creates the exact
+  // combination #1405 is meant to prevent. Contradictory users are skipped
+  // (never silently written) and named in an error toast so the admin knows
+  // which ones were left unchanged.
+  const applicable: { userId: string; groups: string[] }[] = [];
+  const skipped: string[] = [];
+  for (const userId of selectedUserIds) {
+    const user = allUsers.find(u => u.id === userId);
+    if (!user) continue;
+
+    const updatedGroups = [...new Set([...user.groups, groupId])];
+    if (validateGroupCombination(updatedGroups, availableGroups)) {
+      skipped.push(user.email);
+    } else {
+      applicable.push({ userId, groups: updatedGroups });
+    }
+  }
+
+  if (skipped.length > 0) {
+    showError(
+      `Skipped ${skipped.length} user(s) whose group set would be contradictory: ` +
+      `${skipped.join(', ')}. A view-only group cannot be combined with ` +
+      `write-capable groups. Remove the conflicting membership first.`
+    );
+  }
 
-      const updatedGroups = [...new Set([...user.groups, groupId])];
-      await api.updateUser(userId, { groups: updatedGroups });
-    });
+  // Nothing safe to apply -- leave the selection intact so the admin can
+  // adjust the offending memberships and retry.
+  if (applicable.length === 0) return;
 
-    await Promise.all(updates);
+  try {
+    await Promise.all(
+      applicable.map(({ userId, groups }) => api.updateUser(userId, { groups })),
+    );
 
-    clearSelectedUserIds();
+    // Deselect only the successfully-applied users so any skipped
+    // (contradictory) users stay selected for the admin to fix.
+    applicable.forEach(({ userId }) => removeSelectedUserId(userId));
     await loadUsers();
-    showSuccess(`Successfully added ${count} user(s) to group`);
+    showSuccess(`Successfully added ${applicable.length} user(s) to group`);
   } catch (error) {
     console.error('Failed to add users to group:', error);
     showError('Failed to add some users to group');

frontend/src/users/userActions.ts

@@ -14,7 +14,7 @@ import {
   clearSelectedUserIds,
   setAllUsers,
 } from './state';
-import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError } from './utils';
+import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError, validateGroupCombination } from './utils';
 import { openEditUserModal, deleteUser } from './userActions';
 import { ADMINISTRATORS_GROUP_ID } from '../permissions';
 
@@ -210,6 +210,9 @@ function renderUserExpandPanel(user: APIUser): string {
 /**
  * Toggle a group membership for a user inline (no modal).
  *
+ * Before the API call, validates that the resulting group set is not a
+ * contradictory combination (issue #1405: view-only + write-capable).
+ *
  * After a successful API call, patch state in-memory and refresh only the
  * affected expand panel rather than re-rendering the whole table. This
  * prevents the panel from collapsing on every toggle (issue #998) and
@@ -226,6 +229,20 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean
     ? [...new Set([...user.groups, groupId])]
     : user.groups.filter(g => g !== groupId);
 
+  // Validate the resulting group combination before hitting the API.
+  // A view-only group (all permissions are view:*) combined with a
+  // write-capable group is semantically contradictory (issue #1405).
+  const combinationError = validateGroupCombination(next, availableGroups);
+  if (combinationError) {
+    showError(combinationError);
+    // Revert the checkbox immediately so the DOM reflects reality.
+    const cb = document.querySelector<HTMLInputElement>(
+      `.group-assign-checkbox[data-user-id="${userId}"][data-group-id="${groupId}"]`,
+    );
+    if (cb) cb.checked = !checked;
+    return;
+  }
+
   try {
     await api.updateUser(userId, { groups: next });
 
@@ -276,6 +293,9 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean
  */
 function setupUserTableListeners(): void {
   // Select all checkbox
+  // Patch all rows in-place instead of calling renderUsers(), which would
+  // tear down and rebuild the entire table and collapse any open expand
+  // panels (issue #1404 symptom: expand arrow / panel lost on checkbox click).
   const selectAllCheckbox = document.getElementById('select-all-users') as HTMLInputElement;
   if (selectAllCheckbox) {
     selectAllCheckbox.addEventListener('change', (e) => {
@@ -285,24 +305,56 @@ function setupUserTableListeners(): void {
       } else {
         clearSelectedUserIds();
       }
-      renderUsers(filteredUsers);
+      // Patch each row's selected class and its individual checkbox without
+      // re-rendering the whole table (preserves open expand panels).
+      filteredUsers.forEach(user => {
+        const row = document.querySelector<HTMLElement>(
+          `tr.user-row[data-user-id="${user.id}"]`,
+        );
+        const cb = document.querySelector<HTMLInputElement>(
+          `.user-checkbox[data-user-id="${user.id}"]`,
+        );
+        if (row) {
+          if (checked) row.classList.add('row-selected');
+          else row.classList.remove('row-selected');
+        }
+        if (cb) cb.checked = checked;
+      });
       updateBulkActionsBar();
     });
   }
 
   // Individual checkboxes
+  // Same rationale: patch the affected row in-place instead of calling
+  // renderUsers() so open expand panels are not collapsed (issue #1404).
   document.querySelectorAll('.user-checkbox').forEach(checkbox => {
     checkbox.addEventListener('change', (e) => {
       const userId = (e.target as HTMLElement).dataset.userId;
       if (!userId) return;
 
-      if ((e.target as HTMLInputElement).checked) {
+      const nowChecked = (e.target as HTMLInputElement).checked;
+      if (nowChecked) {
         addSelectedUserId(userId);
       } else {
         removeSelectedUserId(userId);
       }
 
-      renderUsers(filteredUsers);
+      // Toggle the row-selected class on just the affected row.
+      const row = document.querySelector<HTMLElement>(
+        `tr.user-row[data-user-id="${userId}"]`,
+      );
+      if (row) {
+        if (nowChecked) row.classList.add('row-selected');
+        else row.classList.remove('row-selected');
+      }
+
+      // Keep the select-all checkbox in sync.
+      const selectAll = document.getElementById('select-all-users') as HTMLInputElement | null;
+      if (selectAll) {
+        selectAll.checked =
+          selectedUserIds.size > 0 && selectedUserIds.size === filteredUsers.length;
+      }
+
       updateBulkActionsBar();
     });
   });

frontend/src/users/userList.ts

@@ -14,7 +14,7 @@ import {
   setCurrentEditingUser,
   availableGroups
 } from './state';
-import { escapeHtml, showError, showSuccess } from './utils';
+import { escapeHtml, showError, showSuccess, validateGroupCombination } from './utils';
 import { loadUsers } from './userActions';
 import { openModal, closeModal } from '../modal';
 
@@ -119,6 +119,14 @@ export async function saveUser(e: Event): Promise<void> {
     return;
   }
 
+  // Reject contradictory group combinations (view-only + write-capable)
+  // before hitting the API so the user gets an actionable message.
+  const groupConflict = validateGroupCombination(selectedGroups, availableGroups);
+  if (groupConflict) {
+    showError(groupConflict);
+    return;
+  }
+
   try {
     if (currentEditingUser) {
       // Update existing user

frontend/src/users/userModals.ts

@@ -19,8 +19,61 @@ export { formatRelativeTime, formatDate } from '../utils';
  */
 export { escapeHtml } from '../utils';
 
+import type { APIGroup } from '../api';
 import { showToast } from '../toast';
 
+/**
+ * Validate that a proposed group-ID list does not contain a contradictory
+ * combination: a view-only group (all permissions carry action === 'view')
+ * paired with a write-capable group (at least one permission has a
+ * non-view action such as create/update/delete/execute/approve/admin).
+ *
+ * Because CUDly's permission model is additive (the user gets the union of
+ * all group permissions), pairing a view-only group with a write group is
+ * semantically contradictory: the intent of a "Viewer" group is that the
+ * user should have read-only access, but adding any write-capable group
+ * silently elevates them.  The UI should surface this immediately rather
+ * than letting the combination go through.
+ *
+ * Groups with zero permissions are skipped (they grant nothing and do not
+ * participate in the conflict).
+ *
+ * Returns a human-readable error string when the combination is invalid,
+ * or null when it is acceptable.
+ *
+ * @param groupIds   UUIDs of groups the user would be assigned
+ * @param allGroups  Full list of loaded groups (from availableGroups)
+ */
+export function validateGroupCombination(
+  groupIds: string[],
+  allGroups: APIGroup[],
+): string | null {
+  const groups = groupIds
+    .map(id => allGroups.find(g => g.id === id))
+    .filter((g): g is APIGroup => g !== undefined);
+
+  const viewOnlyGroups = groups.filter(
+    g => Array.isArray(g.permissions) &&
+         g.permissions.length > 0 &&
+         g.permissions.every(p => p.action === 'view'),
+  );
+  const writeGroups = groups.filter(
+    g => Array.isArray(g.permissions) &&
+         g.permissions.some(p => p.action !== 'view'),
+  );
+
+  if (viewOnlyGroups.length === 0 || writeGroups.length === 0) return null;
+
+  const viewNames = viewOnlyGroups.map(g => `"${g.name}"`).join(', ');
+  const writeNames = writeGroups.map(g => `"${g.name}"`).join(', ');
+  return (
+    `Contradictory group combination: ${viewNames} ` +
+    `${viewOnlyGroups.length === 1 ? 'is' : 'are'} view-only but ` +
+    `${writeNames} grant${writeGroups.length === 1 ? 's' : ''} write permissions. ` +
+    `Assign a view-only group or write-capable groups, not both.`
+  );
+}
+
 /**
  * Show error message. Delegates to the shared toast system so callers
  * across the codebase get consistent bottom-right stacked toasts with

frontend/src/users/utils.ts

@@ -279,15 +279,15 @@ func (m *mockConfigStore) TransitionExecutionStatus(ctx context.Context, executi
 	return nil, nil
 }
 
-func (m *mockConfigStore) SetCancelledBy(_ context.Context, _ string, _ string) error {
+func (m *mockConfigStore) SetCanceledBy(_ context.Context, _ string, _ string) error {
 	return nil
 }
 
-func (m *mockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) {
+func (m *mockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, canceledBy *string) (bool, string, error) {
 	return false, "", nil
 }
 
-func (m *mockConfigStore) CancelScheduledExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) {
+func (m *mockConfigStore) CancelScheduledExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, canceledBy *string) (bool, string, error) {
 	return false, "", nil
 }
 

internal/analytics/collector_test.go

@@ -332,7 +332,7 @@ func TestHandler_rejectRIExchange_AlreadyProcessed(t *testing.T) {
 	mockStore.On("GetRIExchangeRecord", ctx, "11111111-1111-1111-1111-111111111111").Return(
 		&config.RIExchangeRecord{ID: "11111111-1111-1111-1111-111111111111", ApprovalToken: "tok"}, nil)
 	// Transition returns nil indicating already processed
-	mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "cancelled", mock.Anything).
+	mockStore.On("TransitionRIExchangeStatus", ctx, "11111111-1111-1111-1111-111111111111", "pending", "canceled", mock.Anything).
 		Return(nil, nil)
 
 	h := &Handler{config: mockStore}

internal/api/coverage_extras_test.go

@@ -114,10 +114,12 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR
 // wave-2) appear in the History view with a Revoke button before the cloud SDK
 // call fires. Without this entry the row is invisible to the History UI, making
 // the Revoke button unreachable (issue #290, second-wave CR Finding E).
-// historyExecutionStatuses includes both "canceled" (new canonical spelling) and
-// "cancelled" (DB-stored value written by CancelExecutionAtomic until migration
-// #1277 renames the column). Both spellings must be accepted until that migration lands.
-var historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", "canceled", "cancelled"}
+// Both the US-spelling status (config.StatusCanceled) and the legacy British
+// spelling (config.LegacyStatusCanceled) are listed: during the expand-contract
+// rename (migration 000078) old code may still write the legacy value before
+// the rolling deploy completes. The contract migration (#1278) normalizes the
+// data once the deploy is verified stable; drop the legacy entry here then.
+var historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", config.StatusCanceled, config.LegacyStatusCanceled}
 
 // approvalExpiryWindow is how long a pending approval stays actionable
 // before the History view flips it to "expired". Aligns with the
@@ -332,7 +334,10 @@ func annotateHistoryRowByStatus(row *config.PurchaseHistoryRecord, exec config.P
 		row.StatusDescription = exec.Error
 	case "expired":
 		row.StatusDescription = "approval link expired (not approved within 7 days)"
-	case "canceled", "cancelled": // both spellings until migration #1277 renames the DB column
+	case config.StatusCanceled, config.LegacyStatusCanceled:
+		// The legacy British spelling is still matched during the
+		// expand-contract rename (migration 000078) until the contract
+		// migration (#1278) normalizes and drops it.
 		annotateCancelled(row, exec, approver)
 	default:
 		// In-flight (approved/running/scheduled/paused) and audit-gap
@@ -398,15 +403,15 @@ func annotateInFlightOrAuditGapRow(row *config.PurchaseHistoryRecord, exec confi
 }
 
 // annotateCancelled resolves who canceled the execution:
-//  1. exec.CancelledBy — populated by the session-authed deep-link flow;
+//  1. exec.CanceledBy -- populated by the session-authed deep-link flow;
 //     exact session-authed click attribution.
-//  2. approver — the notification inbox that received the cancel token;
+//  2. approver -- the notification inbox that received the cancel token;
 //     authoritative accountable party but not necessarily the clicker.
 //     Used on legacy token-only paths (async workers, old email clicks).
 func annotateCancelled(row *config.PurchaseHistoryRecord, exec config.PurchaseExecution, approver string) {
-	if exec.CancelledBy != nil && *exec.CancelledBy != "" {
-		row.Approver = *exec.CancelledBy
-		row.StatusDescription = "canceled by " + *exec.CancelledBy
+	if exec.CanceledBy != nil && *exec.CanceledBy != "" {
+		row.Approver = *exec.CanceledBy
+		row.StatusDescription = "canceled by " + *exec.CanceledBy
 		return
 	}
 	if approver != "" {
@@ -1009,10 +1014,14 @@ func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistoryS
 		case "expired":
 			summary.TotalExpired++
 			continue
-		case "canceled", "cancelled": // both spellings until migration #1277 renames the DB column
+		case config.StatusCanceled, config.LegacyStatusCanceled:
 			// A canceled purchase represents zero committed spend and zero
 			// realized savings (issue #736). Exclude from all dollar KPIs and
-			// from TotalCompleted — the money was never committed.
+			// from TotalCompleted -- the money was never committed. The legacy
+			// British spelling is matched alongside the US one during the
+			// expand-contract rename (migration 000078) so legacy rows can't
+			// inflate KPIs mid-deploy; the contract migration (#1278) drops it
+			// once the deploy is stable.
 			continue
 		}
 		summary.TotalCompleted++

internal/api/handler_history.go

@@ -1708,10 +1708,17 @@ func TestSummarizePurchaseHistory_CancelledExcludedFromKPIs(t *testing.T) {
 		{Status: "", UpfrontCost: 50.0, EstimatedSavings: 5.0}, // legacy row, no status
 		// One pending row that should be counted as pending, not completed.
 		{Status: "pending", UpfrontCost: 999.0, EstimatedSavings: 99.0},
-		// Two canceled rows — the regression case from issue #736.
-		// Neither must appear in the dollar KPIs or TotalCompleted.
-		{Status: "canceled", UpfrontCost: 500.0, EstimatedSavings: 50.0},
-		{Status: "canceled", UpfrontCost: 750.0, EstimatedSavings: 75.0},
+		// Two canceled rows — the regression case from issue #736. Neither must
+		// appear in the dollar KPIs or TotalCompleted. One uses the new US
+		// spelling (config.StatusCanceled) and one the legacy British spelling
+		// (config.LegacyStatusCanceled): during the expand-contract rename
+		// (migration 000078) a mixed fleet still emits the legacy spelling, so
+		// the dual-spelling read path must exclude BOTH. The constant carries
+		// the legacy value without a literal the US-locale misspell linter would
+		// flag (and without a nolint). Drop the legacy fixture once the contract
+		// migration (#1278) normalizes the data.
+		{Status: config.StatusCanceled, UpfrontCost: 500.0, EstimatedSavings: 50.0},
+		{Status: config.LegacyStatusCanceled, UpfrontCost: 750.0, EstimatedSavings: 75.0},
 	}
 
 	summary := summarizePurchaseHistory(purchases)