Improve edit user UI for auditors

[Luke-Oldenburg]

Summary of the problem

Buttons and form elements weren't disabled for auditors.

Describe your changes

[claude]

Code Review

Thank you for working on improving the auditor experience! I've reviewed the changes and have several concerns that need to be addressed.

🔴 Critical Issues

1. Security Vulnerability: Policy Check Mismatch (app/views/users/edit.html.erb:124)

<% disabled = !policy(@user).update? %>

Problem: The update? policy method checks if the user is an admin OR the record owner:

def update?
  user.admin? || record == user
end

However, the view should check if auditors can edit, which is defined in the edit? policy:

def edit?
  user.auditor? || record == user
end

Impact: This means auditors can view the edit page (authorized by edit?) but all form fields will be disabled because auditors are not admins. This creates a poor UX and doesn't match the PR's stated goal of "improving edit user UI for auditors."

Recommendation: Either:

• Use policy(@user).edit? instead of policy(@user).update? to match the authorization pattern, OR
• Create a new policy method like update_profile? that explicitly defines who can update user profiles

2. Removed Onboarding Functionality Without Explanation (app/views/users/edit.html.erb:132-136, 151-155)

The PR removes several onboarding-specific UX elements:

• Line 132-136: Removed "Complete your profile to get started" message
• Line 151-155: Removed explanatory text for phone number field
• Line 251: Changed submit button from conditional text to always "Save settings"

Problem: The @onboarding variable is still set in the controller (line 129: set_onboarding), but the view no longer uses it for UX customization. This breaks the onboarding flow's user experience.

Impact: New users going through onboarding will see the same generic interface as existing users, losing helpful contextual guidance.

Recommendation: Either:

• Revert the onboarding text removals, OR
• Remove the set_onboarding call from the controller if onboarding is intentionally being removed (and explain why in the PR description)

---

⚠️ Medium Priority Issues

3. Inconsistent UX: Showing Discord Section Outside Onboarding (app/views/users/edit.html.erb:222-228)

<% if @user.has_discord_account? %>
  <div class="card max-w-6xl">
    <!-- Discord account info -->
  </div>
<% end %>

Before: Discord section only shown outside of onboarding flow
After: Always shown if user has a Discord account

Issue: While this might be intentional, the change is not explained in the PR description. This could be a side effect of removing unless @onboarding guards rather than a deliberate improvement.

4. Inconsistent UX: Usage Data Section (app/views/users/edit.html.erb:230-244)

Similar to #3, the usage data sharing fieldset is now shown during onboarding when it was previously hidden.

---

✅ Positive Changes

1. Good refactoring: Using the teenager? method (line 183) instead of inline calculation improves code maintainability
2. Code cleanup: Removing unnecessary blank line (line 122) improves readability
3. Consistent approach: Using a single disabled variable is a good pattern for managing form state

---

🧪 Test Coverage

Missing: No automated tests were added to verify:

• Auditors see disabled form fields when viewing other users' profiles
• Form submission behavior when fields are disabled
• The distinction between edit? and update? permissions

Recommendation: Add system/integration tests covering:

test "auditor cannot edit another user's profile" do
  # Visit edit page as auditor -> should be authorized
  # Try to submit form -> should be rejected by update? policy
end

---

📋 Summary

Before merging, please address:

1. ✅ Fix the policy check mismatch (use edit? or create appropriate policy)
2. ✅ Clarify and fix the onboarding flow changes
3. ✅ Update PR description to explain all intentional behavior changes
4. ✅ Consider adding test coverage for the auditor permission flows

Questions for clarification:

• Is the removal of onboarding-specific text intentional?
• Should auditors be able to update user profiles, or only view them?
• Are the Discord and usage data sections intentionally being shown during onboarding now?

Happy to discuss any of these points further! 👍

[claude]

Code Review for PR 11895: Improve edit user UI for auditors

SUMMARY
This PR improves the user edit form by properly disabling fields for auditors who can view but not edit user profiles.

STRENGTHS

1. Correct Policy Usage: Uses policy(@user).update? to integrate with Pundit authorization (app/policies/user_policy.rb:72-74)
2. Code Cleanup: Removes unnecessary @onboarding conditionals
3. Consistency: All form inputs are consistently disabled when user lacks update permissions
4. Improved Readability: Uses @user.teenager? method instead of inline date comparison (app/models/user.rb:368-370)

POTENTIAL ISSUES

• Variable Naming: The 'disabled' variable conflicts with disabled: parameter. Recommend using 'can_edit' or 'readonly' instead for clarity.
• Birthday Field: Test date_select dropdowns to ensure they are properly disabled and values aren't submitted.

SECURITY
Authorization is correctly enforced at controller level (app/controllers/users_controller.rb:261-265). Even if users bypass disabled fields via dev tools, the controller will reject unauthorized updates. Consider adding a visual banner for auditors explaining read-only mode.

PERFORMANCE
No performance impact. Policy check is cached by Pundit, and removing @onboarding conditionals simplifies rendering.

TEST COVERAGE
Missing test coverage. Recommend adding view/system tests to verify fields are disabled for auditors, and policy tests for UserPolicy#update?.

ADDITIONAL RECOMMENDATIONS

1. Add visual feedback banner for read-only mode
2. Update submit button text or hide for auditors
3. Ensure accessibility with aria-disabled attributes
4. Manually test as auditor, regular user, and admin before merging

VERDICT
Approval with minor suggestions. Core logic is sound and properly implements authorization flow. Main improvements: clearer variable naming and test coverage. Security model is correct. Great work identifying and fixing this UX issue!