Skip to content

[fix] Added SocialAccount inline to UserAdmin when needed#507

Merged
nemesifier merged 3 commits intomasterfrom
fixed-social-account-inline
Apr 25, 2026
Merged

[fix] Added SocialAccount inline to UserAdmin when needed#507
nemesifier merged 3 commits intomasterfrom
fixed-social-account-inline

Conversation

@nemesifier
Copy link
Copy Markdown
Member

If SOCIALACCOUNT_ADMIN_NEEDED is True, admins should be able to manage the social account records of users.

Reference to Existing Issue

Related to #501.

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • N/A I have updated the documentation.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

Warning

Rate limit exceeded

@nemesifier has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 29 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 29 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ba2921db-d02f-4722-9377-04605c197ce3

📥 Commits

Reviewing files that changed from the base of the PR and between 815ff68 and 82d9e63.

📒 Files selected for processing (4)
  • openwisp_users/accounts/urls.py
  • openwisp_users/admin.py
  • openwisp_users/tests/test_admin.py
  • tests/openwisp2/settings.py
📝 Walkthrough

Walkthrough

The changes refactor the admin configuration for Django-allauth socialaccount models. The unregistration logic is reorganized to always unregister SocialToken and SocialAccount, conditionally unregister SocialApp based on the SOCIALACCOUNT_ADMIN_NEEDED setting, and when OAuth/SAML is enabled with admin required, adds a read-only SocialAccountInline to the user admin interface. Additionally, the Google social account provider is added to the test Django settings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Bug Fixes ❌ Error The PR fixes the feature requirement but has critical bugs: SocialAccount imported from wrong module, has_add_permission lacks obj=None default, and unconditional unregister loop will crash when SOCIALACCOUNT_ENABLED is False. No regression tests included. Import SocialAccount from allauth.socialaccount.models, add obj=None default to has_add_permission, and wrap unregister code in SOCIALACCOUNT_ENABLED guard. Add regression tests for the inline functionality.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title includes the required [fix] prefix and clearly describes the main change: adding SocialAccount inline to UserAdmin when a condition is met.
Description check ✅ Passed The description covers the main purpose, references issue #501, and includes all required checklist items with clear status indicators for each item.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fixed-social-account-inline

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openwisp-companion
Copy link
Copy Markdown

openwisp-companion Bot commented Apr 23, 2026

Test Failures in CI

Hello @nemesifier,
(Analysis for commit e5ef526)

There are 3 test failures in the CI:

  1. test_change_user failed with AssertionError: 1 != 0: The test expected the response not to contain "Please correct the error below.", but it did. This indicates an unexpected validation error occurred during the user change process.
  2. test_delete_inline_org_user failed with AssertionError: 1 != 0: The test asserted that the count of a query set should be 0, but it was 1. This suggests that an organization user was not deleted as expected.
  3. test_delete_inline_owner_org_user failed with AssertionError: False is not true: The test asserted that a specific message ("t delete 1 organization user because it belongs to an organization owner.") should be present in the response, but it was not found. This points to an issue with the deletion logic or the error message handling.
  4. test_update_user_no_validation_error failed with AssertionError: 'user1' != 'user2': The test expected the user's username to be 'user2' after an update, but it remained 'user1'. This indicates that the user update operation did not change the username as expected.

To address these failures, please review the test cases and the corresponding code logic for user modification and deletion, particularly in the openwisp_users.tests.test_admin module. Pay close attention to how organization users are handled during deletion and how user updates are processed.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@openwisp_users/admin.py`:
- Around line 682-683: Update the has_add_permission method signature to accept
an optional obj by changing def has_add_permission(self, request, obj): to
include a default (obj=None) so it matches Django's InlineModelAdmin signature
and aligns with the neighboring has_delete_permission(self, request, obj=None);
ensure only the signature is changed and behavior (returning False) remains the
same.
- Line 675: The import for SocialAccount is using the admin package instead of
the model; replace the incorrect import (from allauth.socialaccount.admin import
SocialAccount) with importing the model from allauth.socialaccount.models so the
SocialAccount symbol refers to the actual model class used in registrations and
admin classes (update the import statement where SocialAccount is referenced,
e.g., in the admin module that defines/administers social accounts).
- Around line 663-694: The unregister logic currently builds
_unregister_socialaccount_models and calls apps.get_model/unregister
unconditionally, causing LookupError when allauth.socialaccount is not
installed; wrap the list initialization and the for loop that calls
apps.get_model and admin.site.unregister inside a guard checking
allauth_settings.SOCIALACCOUNT_ENABLED (same condition that controls the
SocialAccountInline/UserAdmin.inlines), so the _unregister_socialaccount_models
list and the unregister loop only run when
allauth_settings.SOCIALACCOUNT_ENABLED is True.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7f3da497-0cb4-444a-8ca8-5a7d7db8b78a

📥 Commits

Reviewing files that changed from the base of the PR and between 815ff68 and e5ef526.

📒 Files selected for processing (2)
  • openwisp_users/admin.py
  • tests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.0.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.0.0
  • GitHub Check: Python==3.10 | django~=5.0.0
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (python)
  • GitHub Check: Analyze (actions)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework (django.utils.translation.gettext, gettext_lazy, or ugettext aliases)

Files:

  • tests/openwisp2/settings.py
  • openwisp_users/admin.py

Comment thread openwisp_users/admin.py Outdated
Comment thread openwisp_users/admin.py Outdated
Comment thread openwisp_users/admin.py Outdated
If SOCIALACCOUNT_ADMIN_NEEDED is True, admins should be
able to manage the social account records of users.

[backport 1.2]
@nemesifier nemesifier force-pushed the fixed-social-account-inline branch from e5ef526 to 84b5186 Compare April 23, 2026 22:45
@openwisp-companion
Copy link
Copy Markdown

Test Failures in openwisp-users CI

Hello @nemesifier,
(Analysis for commit 84b5186)

The CI failed due to a LookupError: No installed app with label 'socialaccount'. This indicates that the allauth.socialaccount app is not correctly configured or found during the test run.

Fix:

Ensure that allauth.socialaccount is correctly included in the INSTALLED_APPS setting when running tests, especially in the test environment where specific configurations might be applied.

The failure occurs during the execution of tests that run without allauth.socialaccount enabled, specifically when the openwisp_users/admin.py file attempts to access models from this app. The traceback points to openwisp_users/admin.py, line 692, where apps.get_model(*model) is called.

To resolve this, you should ensure that the test setup correctly handles the conditional inclusion or exclusion of allauth.socialaccount and that all necessary apps are available when needed.

If the allauth.socialaccount app is intentionally excluded for certain tests, ensure that any code that relies on it is also conditionally executed or mocked appropriately.

@openwisp-companion
Copy link
Copy Markdown

<h3>Django ImproperlyConfigured: allauth.socialaccount not installed</h3>

Hello @nemesifier,
(Analysis for commit 9334722)

The CI failed because the allauth.socialaccount app was not installed, but its models were being imported. This typically happens when the SAMPLE_APP environment variable is set, and the openwisp_users project tries to include allauth.socialaccount.providers.google in INSTALLED_APPS without allauth.socialaccount itself being present.

Fix:

Ensure that allauth.socialaccount is installed and included in INSTALLED_APPS when SAMPLE_APP is enabled, or if allauth.socialaccount.providers.google is intended to be used.

If allauth.socialaccount is not needed for the tests, you can remove the import of allauth.socialaccount.providers.google from tests/openwisp2/settings.py.

@nemesifier
Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

✅ Actions performed

Full review triggered.

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 97.908% (+0.009%) from 97.899% — fixed-social-account-inline into master

@nemesifier
Copy link
Copy Markdown
Member Author

@coderabbitai resolve

@github-project-automation github-project-automation Bot moved this from In progress to Reviewer approved in OpenWISP Priorities for next releases Apr 25, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 25, 2026

✅ Actions performed

Comments resolved and changes approved.

@nemesifier nemesifier merged commit 7db9735 into master Apr 25, 2026
23 checks passed
@nemesifier nemesifier deleted the fixed-social-account-inline branch April 25, 2026 19:30
@github-project-automation github-project-automation Bot moved this from Reviewer approved to Done in OpenWISP Priorities for next releases Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants