[feature] Add extensible field serialization to export_users command #497#498
[feature] Add extensible field serialization to export_users command #497#498
Conversation
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe export_users management command normalizes configured export fields (strings or dicts) via Sequence Diagram(s)sequenceDiagram
participant CLI as Management Command (export_users)
participant Cmd as Command
participant Norm as normalize_field
participant DB as ORM/Database
participant Resolver as _get_field_value
participant Callable as Field callable
participant Serializer as serialize_related/_get_nested_attr
participant CSV as CSV writer
CLI->>Cmd: run export_users
Cmd->>Norm: load & normalize config (fields, select_related, prefetch_related)
Cmd->>DB: prepare queryset (.select_related/.prefetch_related)
DB-->>Cmd: ready queryset
Cmd->>DB: iterate queryset (iterator(chunk_size=1000)) -> user
loop per user
Cmd->>Resolver: for each normalized field
alt field has callable
Resolver->>Callable: call(user)
Callable-->>Resolver: value or exception
else field has nested fields
Resolver->>Serializer: fetch related manager/attrs -> serialized value
Serializer-->>Resolver: value
else dot-notation attribute
Resolver->>Serializer: resolve attribute chain -> value or ''
Serializer-->>Resolver: value
end
Resolver-->>Cmd: field value
Cmd->>CSV: write row value
end
CSV-->>CLI: output file/stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
pandafy
force-pushed
the
issues/497-export-users
branch
2 times, most recently
from
b9f3cc9 to
45682eb
Compare
pandafy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/management/commands/export_users.py`:
- Around line 19-24: resolve_attr currently documents returning None for missing
attributes but only catches ObjectDoesNotExist, so AttributeError still bubbles
up; update resolve_attr to handle both missing-attribute and DB-missing cases by
either using getattr(obj, attr_name, None) or wrapping getattr in a try/except
that catches (ObjectDoesNotExist, AttributeError) and returns None; reference
resolve_attr, getattr and ObjectDoesNotExist when making the change.
- Around line 105-109: The current exception handler around callable_fn(user)
raises a generic Exception with a non-translated message; change it to raise
django.core.management.CommandError and wrap the message with gettext (e.g.
gettext or alias _) so it is translatable. Update the import list to include
CommandError from django.core.management and gettext from
django.utils.translation, then replace the raise Exception(...) in the except
block inside the function that calls callable_fn (look for callable_fn and the
surrounding try/except) with raise CommandError(_("Error calling function for
field '%s': %s") % (name, e)) or equivalent translatable string formatting.
- Line 33: Remove the use of .iterator() which bypasses
select_related/prefetch_related: replace any manager.iterator() and
RelatedManager.iterator() usages (e.g., the list comprehension creating rows
that uses manager.iterator(), plus the other occurrences referenced) with plain
iteration over the queryset/manager (just manager or queryset) so
prefetching/selection takes effect; if chunked iteration is truly required, use
.iterator(chunk_size=...) explicitly, but prefer removing .iterator() to keep
prefetch_related/select_related benefits in export_users.py (look for the
variables manager, subfields, and the RelatedManager usages to locate the
spots).
🪄 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: 3808624e-0d36-4926-82bb-56f335b220f2
📒 Files selected for processing (4)
docs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/settings.pyopenwisp_users/tests/test_commands.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). (14)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 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:
openwisp_users/settings.pyopenwisp_users/tests/test_commands.pyopenwisp_users/management/commands/export_users.py
🔇 Additional comments (3)
openwisp_users/settings.py (1)
18-23: Good extensibility update for organizations export config.The callable-based
organizationsfield and explicitprefetch_relatedconfig align well with the new generic export pipeline.Also applies to: 41-44
docs/user/settings.rst (1)
114-199: Docs are consistent with the new field normalization and relation export behavior.The updated examples and priority rules clearly reflect the current command configuration model.
openwisp_users/tests/test_commands.py (1)
50-54: Test coverage improvements are solid.The normalized-header assertions, constant-query test, and callable-failure case materially improve confidence in the new export behavior.
Also applies to: 118-143, 145-162
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
Commit Message Format FailureHello @pandafy, The CI failed because the commit message does not adhere to the required format. Fix: Here's an example of the correct format: |
|
coverage: 98.022% (+0.1%) from 97.899% — issues/497-export-users into master |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
openwisp_users/management/commands/export_users.py (3)
19-24:⚠️ Potential issue | 🟡 MinorCatch
AttributeErrorhere too.The docstring says missing attributes return
None, butgetattr()on an unknown field still raises and aborts the export because onlyObjectDoesNotExistis handled.Proposed fix
def resolve_attr(obj, attr_name): """Return the attribute value on obj, or None if it does not exist.""" try: return getattr(obj, attr_name) - except ObjectDoesNotExist: + except (ObjectDoesNotExist, AttributeError): return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 19 - 24, The resolve_attr helper currently only catches ObjectDoesNotExist, so getattr(obj, attr_name) can still raise AttributeError and abort exports; update resolve_attr to also catch AttributeError (in addition to ObjectDoesNotExist) and return None when either exception occurs so missing attributes behave as documented; locate the resolve_attr function and extend its except clause to handle AttributeError for safe attribute resolution.
104-108:⚠️ Potential issue | 🟠 MajorRaise
CommandErrorand translate the failure message.This is a user-facing management-command error, but it is emitted as a generic
Exceptionwith a hard-coded English string. UseCommandErrorand wrap the message withgettext.Proposed fix
-from django.core.management.base import BaseCommand +from django.core.management.base import BaseCommand, CommandError +from django.utils.translation import gettext as _ @@ if callable_fn is not None: try: return callable_fn(user) except Exception as e: - raise Exception(f"Error calling function for field '{name}': {e}") + raise CommandError( + _("Error calling function for field '%(field)s': %(error)s") + % {"field": name, "error": e} + ) from eAs per coding guidelines: 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).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 104 - 108, Replace the generic Exception with Django's CommandError and make the message translatable: import CommandError from django.core.management and gettext (e.g. from django.utils.translation import gettext as _), then change the raise to raise CommandError(_("Error calling function for field '%s': %s") % (name, e)) so the management command surfaces a CommandError and the message is wrapped for i18n; update the exception handling around callable_fn (the block with callable_fn, user, name) accordingly.
27-38:⚠️ Potential issue | 🟠 MajorRemove
.iterator()from the prefetched paths.These iterations bypass Django’s prefetched cache, so the new
prefetch_relatedoption will not actually prevent N+1 queries on the code paths this PR adds.Proposed fix
- rows = [[str(getattr(obj, f, "")) for f in subfields] for obj in manager.iterator()] + rows = [[str(getattr(obj, f, "")) for f in subfields] for obj in manager]- for user in queryset.iterator(): + for user in queryset: csv_writer.writerow( [self._get_field_value(user, field) for field in fields] )- return ",".join( - str(getattr(obj, sub_attr, "")) for obj in intermediate.iterator() - ) + return ",".join( + str(getattr(obj, sub_attr, "")) for obj in intermediate + )Also applies to: 89-93, 124-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 27 - 38, The serialize_related function (and the other similar blocks at the other ranges) is iterating via manager.iterator(), which bypasses Django's prefetched_related cache and causes N+1 queries; change the iteration to use the cached queryset (e.g. iterate over manager.all() or the manager itself) instead of calling .iterator() so the prefetched objects are used (update serialize_related and the analogous loops at the other locations to remove .iterator()).
🤖 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/management/commands/export_users.py`:
- Around line 117-131: The dot-notation handling in export_users.py only splits
the first segment using split(".", 1) causing multi-hop paths like
"registered_user.organization.name" to break; replace the single-split logic in
the block that uses name, model_attr, sub_attr and intermediate with iterative
traversal: split name into segments = name.split(".") and loop through segments
resolving getattr at each hop, catching ObjectDoesNotExist and returning "" when
encountered; if an intermediate object has an "iterator" (related manager) and
there are remaining segments, map the remaining attribute path across each
related object (joining comma-separated string results) rather than trying to
getattr the manager itself; ensure the final resolved value is returned (or ""
when missing) and remove the split(".", 1) usage.
- Around line 64-70: The exclude_fields list uses
options.get("exclude_fields").split(",") which preserves whitespace and empty
tokens, so names like " email" won't match; update the parsing of exclude_fields
(in the export/users command where exclude_fields is defined and used alongside
normalize_field and the fields list comprehension) to strip each token and
discard empty strings (e.g., build exclude_fields = [t.strip() for t in
options.get("exclude_fields").split(",") if t.strip()]) before using
normalize_field(field)["name"] not in exclude_fields to filter fields.
In `@openwisp_users/settings.py`:
- Around line 18-22: The current _export_organizations() uses
user.organizations_dict which forces per-user DB lookups; change it to prefer
serializing from the prefetched relation (e.g., iterate over
user.organizations.all() so prefetch_related results are used) and only fall
back to user.organizations_dict if the relation isn't present, and apply the
same change to the other organizations serializer at the referenced block (lines
~41-44). Update _export_organizations and the second serializer to build the
"(org_id,is_admin)" strings from the prefetched organization objects (or a safe
fallback to organizations_dict) so the built-in organizations column benefits
from bulk prefetching.
In `@openwisp_users/tests/test_commands.py`:
- Around line 118-143: Update the test to exercise the prefetch_related /
related-manager serialization branch by modifying or adding a test similar to
test_related_fields_no_n_plus_1 that sets
app_settings.EXPORT_USERS_COMMAND_CONFIG to include a prefetch_related entry and
a related-field descriptor (e.g. {"name": "groups", "fields": ["name"]} or
similar) instead of only using select_related with "auth_token.key"; then create
related objects (e.g. add users to a group or create multiple related items),
call call_command("export_users", filename=...), assert the query count remains
constant (use assertNumQueries) and verify the exported CSV includes the
serialized related-manager fields for each user so the prefetch path is actually
exercised.
---
Duplicate comments:
In `@openwisp_users/management/commands/export_users.py`:
- Around line 19-24: The resolve_attr helper currently only catches
ObjectDoesNotExist, so getattr(obj, attr_name) can still raise AttributeError
and abort exports; update resolve_attr to also catch AttributeError (in addition
to ObjectDoesNotExist) and return None when either exception occurs so missing
attributes behave as documented; locate the resolve_attr function and extend its
except clause to handle AttributeError for safe attribute resolution.
- Around line 104-108: Replace the generic Exception with Django's CommandError
and make the message translatable: import CommandError from
django.core.management and gettext (e.g. from django.utils.translation import
gettext as _), then change the raise to raise CommandError(_("Error calling
function for field '%s': %s") % (name, e)) so the management command surfaces a
CommandError and the message is wrapped for i18n; update the exception handling
around callable_fn (the block with callable_fn, user, name) accordingly.
- Around line 27-38: The serialize_related function (and the other similar
blocks at the other ranges) is iterating via manager.iterator(), which bypasses
Django's prefetched_related cache and causes N+1 queries; change the iteration
to use the cached queryset (e.g. iterate over manager.all() or the manager
itself) instead of calling .iterator() so the prefetched objects are used
(update serialize_related and the analogous loops at the other locations to
remove .iterator()).
🪄 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: ea1b5758-1b5e-4dc9-94a7-bdc7548bb71e
📒 Files selected for processing (4)
docs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/settings.pyopenwisp_users/tests/test_commands.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). (16)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 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:
openwisp_users/settings.pyopenwisp_users/tests/test_commands.pyopenwisp_users/management/commands/export_users.py
🧠 Learnings (2)
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
pandafy
force-pushed
the
issues/497-export-users
branch
from
45682eb to
d9c6199
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
openwisp_users/management/commands/export_users.py (3)
119-123:⚠️ Potential issue | 🟠 MajorUsing
.iterator()on RelatedManager bypasses prefetch_related benefits.Line 122 calls
intermediate.iterator()on a RelatedManager, which executes a fresh query and ignores the prefetched cache. This negates the N+1 optimization when usingprefetch_relatedfor reverse relations accessed via dot notation.🔧 Proposed fix
if hasattr(intermediate, "iterator"): # Related manager accessed via dot notation → comma-separated values return ",".join( - str(getattr(obj, sub_attr, "")) for obj in intermediate.iterator() + str(getattr(obj, sub_attr, "")) for obj in intermediate.all() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 119 - 123, The current code calls intermediate.iterator() on a RelatedManager (variable intermediate) which bypasses any prefetched cache; replace the iterator() call with iteration over intermediate.all() (or simply intermediate) so that prefetch_related results are used: update the generator expression in export_users/management/commands/export_users.py to iterate over intermediate.all() (or the manager directly) instead of intermediate.iterator(), preserving the comma-joined string behavior.
56-62:⚠️ Potential issue | 🟡 MinorWhitespace in
--exclude-fieldsvalues will cause matching failures.
split(",")preserves whitespace, so--exclude-fields="id, email"produces["id", " email"]and" email"won't match"email".🔧 Proposed fix
- exclude_fields = options.get("exclude_fields").split(",") + exclude_fields = [ + f.strip() for f in options.get("exclude_fields", "").split(",") if f.strip() + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 56 - 62, The exclude_fields list produced by options.get("exclude_fields").split(",") can contain leading/trailing whitespace so matches against normalized field names fail; trim whitespace and ignore empty entries by mapping strip() and filtering out falsy values when constructing exclude_fields before the list comprehension that filters fields (refer to options.get("exclude_fields"), exclude_fields, normalize_field, and the fields list comprehension) so comparisons use cleaned names like "email" instead of " email".
96-100:⚠️ Potential issue | 🟠 MajorRaise
CommandErrorwith a translatable message.The exception at line 100 uses a generic
Exceptionwith a user-facing string that is not marked for translation. Management commands should useCommandError, and per Django i18n guidelines, user-facing strings must usegettext.🔧 Proposed fix
+from django.core.management.base import BaseCommand, CommandError +from django.utils.translation import gettext as _ # In _get_field_value: if callable_fn is not None: try: return callable_fn(user) except Exception as e: - raise Exception(f"Error calling function for field '{name}': {e}") + raise CommandError( + _("Error calling function for field '%(field)s': %(error)s") + % {"field": name, "error": e} + ) from eBased on learnings: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 96 - 100, The current exception raised when callable_fn(user) fails uses a plain Exception and an untranslated message; change this to raise django.core.management.CommandError with a translatable message using gettext (e.g. from django.utils.translation import gettext as _). Update the except block in export_users.py (the callable_fn handling) to catch Exception as e and raise CommandError(_("Error calling function for field '%(name)s': %(err)s") % {"name": name, "err": str(e)}), and add the necessary imports for CommandError and gettext at the top of the file.
🤖 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/management/commands/export_users.py`:
- Around line 112-127: The current dot-notation handling in export_users (the
split(".", 1) that produces model_attr and sub_attr and uses intermediate) only
supports two-level paths; update the logic to iterate through all path segments
instead of a single split so paths like registered_user.organization.name
traverse each attribute in turn (handle related managers/iterables at any level
by checking hasattr(intermediate, "iterator") and joining values when
encountered), or if you choose not to implement traversal, add a clear
comment/docstring near the dot-notation handling explaining the deliberate
two-level limitation (referencing split(".", 1), model_attr, sub_attr, and
intermediate).
In `@openwisp_users/tests/test_commands.py`:
- Around line 164-190: The decorator for `@patch.object` setting
app_settings.EXPORT_USERS_COMMAND_CONFIG references app_label (e.g.,
f"{app_label}_organizationuser") which is not in scope at decoration time; fix
by replacing those f-strings with the actual literal string
"openwisp_users_organizationuser" (or move app_label to module-level and
reference that constant) so the decorator no longer relies on a class attribute;
update the entries in the EXPORT_USERS_COMMAND_CONFIG fields list and any
duplicated manager entries accordingly.
---
Duplicate comments:
In `@openwisp_users/management/commands/export_users.py`:
- Around line 119-123: The current code calls intermediate.iterator() on a
RelatedManager (variable intermediate) which bypasses any prefetched cache;
replace the iterator() call with iteration over intermediate.all() (or simply
intermediate) so that prefetch_related results are used: update the generator
expression in export_users/management/commands/export_users.py to iterate over
intermediate.all() (or the manager directly) instead of intermediate.iterator(),
preserving the comma-joined string behavior.
- Around line 56-62: The exclude_fields list produced by
options.get("exclude_fields").split(",") can contain leading/trailing whitespace
so matches against normalized field names fail; trim whitespace and ignore empty
entries by mapping strip() and filtering out falsy values when constructing
exclude_fields before the list comprehension that filters fields (refer to
options.get("exclude_fields"), exclude_fields, normalize_field, and the fields
list comprehension) so comparisons use cleaned names like "email" instead of "
email".
- Around line 96-100: The current exception raised when callable_fn(user) fails
uses a plain Exception and an untranslated message; change this to raise
django.core.management.CommandError with a translatable message using gettext
(e.g. from django.utils.translation import gettext as _). Update the except
block in export_users.py (the callable_fn handling) to catch Exception as e and
raise CommandError(_("Error calling function for field '%(name)s': %(err)s") %
{"name": name, "err": str(e)}), and add the necessary imports for CommandError
and gettext at the top of the file.
🪄 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: 4a1ba6e9-50e0-4163-9aaa-30dad6a149aa
📒 Files selected for processing (4)
docs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/settings.pyopenwisp_users/tests/test_commands.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.13 | 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.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.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:
openwisp_users/settings.pyopenwisp_users/tests/test_commands.pyopenwisp_users/management/commands/export_users.py
🧠 Learnings (2)
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
🔇 Additional comments (6)
openwisp_users/settings.py (1)
18-22: The_export_organizationscallable still triggers per-user database queries.The function accesses
user.organizations_dict, which (peropenwisp_users/base/models.py:128-155) executes a query scoped to a single user. Withprefetch_related: []at line 44, the N+1 mitigation introduced by this PR is not leveraged for the built-inorganizationscolumn.Consider iterating over
user.openwisp_users_organizationuser_set.all()(or the appropriate reverse relation) and adding that relation toprefetch_relatedso the prefetched cache is used.docs/user/settings.rst (1)
84-198: LGTM!The documentation comprehensively covers the new field definition schema, priority rules, related object handling, and query optimization options with clear examples.
openwisp_users/tests/test_commands.py (2)
145-162: LGTM on error handling test coverage.The test correctly validates that callable errors propagate with the expected message format
"Error calling function for field 'broken'".
220-234: LGTM on ObjectDoesNotExist edge case test.Good unit test coverage for the dot-notation path when sub-attribute access raises
ObjectDoesNotExist.openwisp_users/management/commands/export_users.py (2)
12-16: LGTM on helper functions.
normalize_fieldandserialize_relatedare clean implementations.serialize_relatedcorrectly uses.all()to leverage prefetched results.Also applies to: 19-30
82-85: No changes needed — iterator with chunk_size and prefetch_related is appropriate.The code correctly uses
prefetch_related()beforeiterator(chunk_size=1000). Django 4.2+ (the minimum version tested in CI) fully supports this pattern: prefetched objects load per chunk as designed, making it well-suited for memory-efficient bulk exports.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
openwisp_users/management/commands/export_users.py (3)
121-125:⚠️ Potential issue | 🟠 MajorReuse the prefetched related manager here.
prefetch_related()populates the related manager's cached queryset and Django reuses that viarelated_manager.all().QuerySet.iterator()is a direct database iterator, sointermediate.iterator()bypasses the prefetched cache and can reintroduce one query per exported user. (docs.djangoproject.com)⚡ Suggested fix
- return ",".join( - str(getattr(obj, sub_attr, "")) for obj in intermediate.iterator() - ) + return ",".join( + str(getattr(obj, sub_attr, "")) for obj in intermediate.all() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 121 - 125, The current export loop uses intermediate.iterator(), which bypasses prefetch_related cache and can issue extra queries; change it to iterate the prefetched related manager by calling intermediate.all() (i.e., replace intermediate.iterator() with intermediate.all()) so the generator expression using getattr(obj, sub_attr, "") reuses the cached queryset from the related manager.
98-103:⚠️ Potential issue | 🟠 MajorTranslate the new
CommandErrormessage.This is a new user-facing Django command string, but it is still hard-coded English.
As per coding guidelines, "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)".🌐 Suggested fix
from django.core.management.base import BaseCommand, CommandError +from django.utils.translation import gettext as _ @@ try: return callable_fn(user) except Exception as e: - raise CommandError(f"Error calling function for field '{name}': {e}") + raise CommandError( + _("Error calling function for field '%(field)s': %(error)s") + % {"field": name, "error": e} + ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 98 - 103, The CommandError message raised in the callable_fn branch is a new user-facing string and must be made translatable: import gettext as _ from django.utils.translation (or use gettext_lazy) at the top of the module, then replace the hard-coded f-string in the raise inside the callable_fn block (the code that currently does raise CommandError(f"Error calling function for field '{name}': {e}")) with a translatable message using _(), e.g. use a translation string with placeholders and interpolate name and the exception (e.g. _("Error calling function for field '%(name)s': %(error)s") % {"name": name, "error": e}) so the error text is internationalized.
115-128:⚠️ Potential issue | 🟠 Major
split(".", 1)still limits dot notation to one hop.
registered_user.organization.namebecomesgetattr(registered_user, "organization.name")after the first step, so anything deeper thana.bstill fails. The traversal needs to walk all remaining segments, including when a related manager appears mid-path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 115 - 128, The current dot-traversal only splits once (split(".", 1)) and fails for deeper paths; rewrite the traversal to iterate over all segments (parts = name.split(".")) walking a current value starting from user, at each segment doing getattr and catching ObjectDoesNotExist to return "" if missing; if the current value has an iterator (related manager) while there are remaining segments, collect a comma-separated list by iterating over the manager and using getattr on each object for the remaining joined subpath, otherwise continue walking segments until returning the final attribute value. Ensure you update uses of variables like model_attr/sub_attr/intermediate to the iterative approach and preserve existing exception handling and return types.openwisp_users/tests/test_commands.py (1)
163-199:⚠️ Potential issue | 🟠 MajorAdd a query-budget assertion for the prefetched manager path.
This is the only test that exercises
prefetch_relatedwith related-manager serialization, butcall_command()still isn't wrapped inassertNumQueries. The current manager dot-notation path can regress back to one extra query per user and this test will still pass.Based on learnings: "Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior."🧪 Suggested test tightening
- call_command("export_users", filename=self.temp_file.name) + with self.assertNumQueries(2): + call_command("export_users", filename=self.temp_file.name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/tests/test_commands.py` around lines 163 - 199, The test test_subfields_dict_field doesn't assert query count for the prefetched manager path, so wrap the call_command("export_users", ...) in an assertNumQueries context to ensure openwisp_users_organizationuser is prefetched and no per-user extra queries occur; locate the test_subfields_dict_field function and the EXPORT_USERS_COMMAND_CONFIG patch and add a call to self.assertNumQueries(<expected_count>) (or assertNumQueries from django.test) around call_command("export_users", filename=self.temp_file.name) to lock the expected query budget for the manager dot-notation path.
🤖 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/management/commands/export_users.py`:
- Around line 56-64: The exclusion logic is matching only
normalize_field(field)["name"], which is ambiguous for dict/related fields;
change it to compute and match against a unique field identifier (e.g., the full
normalized path or column key returned by normalize_field such as
normalize_field(field)["path"] or ["key"]) wherever fields are filtered (the
exclude_fields parsing and the list comprehension that builds fields) and use
the same unique identifier when you build CSV headers (the corresponding header
generation code referenced around lines 80-81) so that each column can be
excluded independently; also accept legacy plain names by falling back to
matching the old "name" if no full identifier is provided.
In `@openwisp_users/tests/test_commands.py`:
- Line 13: The test currently imports and uses normalize_field() from
export_users which mirrors the implementation and can hide regressions; remove
the import of normalize_field and stop calling it in the test oracle, and
instead hard-code the expected header names and exclude lists directly in the
assertions (replace uses at the spots around normalize_field, and where Command
output is compared at lines ~50-54, 71-72, 110-114) so the test asserts explicit
expected values independent of the command's normalize_field implementation.
---
Duplicate comments:
In `@openwisp_users/management/commands/export_users.py`:
- Around line 121-125: The current export loop uses intermediate.iterator(),
which bypasses prefetch_related cache and can issue extra queries; change it to
iterate the prefetched related manager by calling intermediate.all() (i.e.,
replace intermediate.iterator() with intermediate.all()) so the generator
expression using getattr(obj, sub_attr, "") reuses the cached queryset from the
related manager.
- Around line 98-103: The CommandError message raised in the callable_fn branch
is a new user-facing string and must be made translatable: import gettext as _
from django.utils.translation (or use gettext_lazy) at the top of the module,
then replace the hard-coded f-string in the raise inside the callable_fn block
(the code that currently does raise CommandError(f"Error calling function for
field '{name}': {e}")) with a translatable message using _(), e.g. use a
translation string with placeholders and interpolate name and the exception
(e.g. _("Error calling function for field '%(name)s': %(error)s") % {"name":
name, "error": e}) so the error text is internationalized.
- Around line 115-128: The current dot-traversal only splits once (split(".",
1)) and fails for deeper paths; rewrite the traversal to iterate over all
segments (parts = name.split(".")) walking a current value starting from user,
at each segment doing getattr and catching ObjectDoesNotExist to return "" if
missing; if the current value has an iterator (related manager) while there are
remaining segments, collect a comma-separated list by iterating over the manager
and using getattr on each object for the remaining joined subpath, otherwise
continue walking segments until returning the final attribute value. Ensure you
update uses of variables like model_attr/sub_attr/intermediate to the iterative
approach and preserve existing exception handling and return types.
In `@openwisp_users/tests/test_commands.py`:
- Around line 163-199: The test test_subfields_dict_field doesn't assert query
count for the prefetched manager path, so wrap the call_command("export_users",
...) in an assertNumQueries context to ensure openwisp_users_organizationuser is
prefetched and no per-user extra queries occur; locate the
test_subfields_dict_field function and the EXPORT_USERS_COMMAND_CONFIG patch and
add a call to self.assertNumQueries(<expected_count>) (or assertNumQueries from
django.test) around call_command("export_users", filename=self.temp_file.name)
to lock the expected query budget for the manager dot-notation path.
🪄 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: 64e1b1c0-7b6f-428f-b8ae-e298642a9639
📒 Files selected for processing (2)
openwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.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). (14)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 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:
openwisp_users/tests/test_commands.pyopenwisp_users/management/commands/export_users.py
🧠 Learnings (2)
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_users/management/commands/export_users.py (2)
118-122:⚠️ Potential issue | 🟡 MinorError message should be marked for translation.
Per Django i18n guidelines, the error message string should use
gettext.Proposed fix
except Exception as e: - raise CommandError(f"Error calling function for field '{name}': {e}") + raise CommandError( + _("Error calling function for field '%(field)s': %(error)s") + % {"field": name, "error": e} + ) from eAs per coding guidelines: "For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 118 - 122, The error message raised in the callable wrapper should be marked for translation: import gettext (e.g. from django.utils.translation import gettext as _) and wrap the CommandError message string with _(), changing the raise in the exception handler in the function that invokes callable_fn (the block referencing callable_fn, user, name and raising CommandError) to use _("Error calling function for field '%s': %s") or similar with formatting using name and the exception; ensure the gettext import is added at the top of the module (export_users.py) so the translated string is used.
104-109:⚠️ Potential issue | 🟠 MajorUsing
.iterator()on RelatedManager bypasses prefetch_related cache.When
currentis a RelatedManager and.iterator()is called, it executes a new database query instead of using the prefetched results cache. This causes N+1 queries when traversing multi-hop dot-notation paths through prefetched relations.Proposed fix
if hasattr(current, "iterator") and i < len(parts) - 1: remaining_path = ".".join(parts[i + 1 :]) return ",".join( str(self._get_nested_attr(item, remaining_path)) - for item in current.iterator() + for item in current.all() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 104 - 109, The code calls current.iterator() inside _get_nested_attr when traversing multi-hop relations, which bypasses prefetch_related cache and causes N+1 queries; change the iteration to use the RelatedManager queryset (e.g., current.all() or simply iter(current.all())) so prefetched results are used, i.e., in the block referencing current, parts, remaining_path and _get_nested_attr replace current.iterator() with current.all() (or list(current.all()) if strict materialization is needed) to avoid making new DB queries.
🤖 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/management/commands/export_users.py`:
- Around line 131-134: The conditional in export_users.py that checks if "." in
name is redundant because both branches call the same helper; simplify by
removing the if/else and directly return self._get_nested_attr(user, name) in
the containing method (referencing the same _get_nested_attr helper and the
local variable name/user) so behavior remains unchanged and the dead branch is
eliminated.
- Around line 64-76: The success message written via
self.stdout.write(self.style.SUCCESS(f"User data exported successfully to
{filename}!")) is not marked for translation; import gettext as _ from
django.utils.translation and wrap the user-facing string passed to
self.style.SUCCESS with _(…) so the message becomes translatable (keeping the
filename interpolation), and ensure the import is added near other imports in
the module.
---
Duplicate comments:
In `@openwisp_users/management/commands/export_users.py`:
- Around line 118-122: The error message raised in the callable wrapper should
be marked for translation: import gettext (e.g. from django.utils.translation
import gettext as _) and wrap the CommandError message string with _(), changing
the raise in the exception handler in the function that invokes callable_fn (the
block referencing callable_fn, user, name and raising CommandError) to use
_("Error calling function for field '%s': %s") or similar with formatting using
name and the exception; ensure the gettext import is added at the top of the
module (export_users.py) so the translated string is used.
- Around line 104-109: The code calls current.iterator() inside _get_nested_attr
when traversing multi-hop relations, which bypasses prefetch_related cache and
causes N+1 queries; change the iteration to use the RelatedManager queryset
(e.g., current.all() or simply iter(current.all())) so prefetched results are
used, i.e., in the block referencing current, parts, remaining_path and
_get_nested_attr replace current.iterator() with current.all() (or
list(current.all()) if strict materialization is needed) to avoid making new DB
queries.
🪄 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: 37dae2c7-95f1-4008-97e7-48235fba4f92
📒 Files selected for processing (2)
openwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.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). (18)
- GitHub Check: CodeQL
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.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:
openwisp_users/tests/test_commands.pyopenwisp_users/management/commands/export_users.py
🧠 Learnings (2)
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
🔇 Additional comments (12)
openwisp_users/tests/test_commands.py (8)
50-70: LGTM! Explicit expected headers avoid mirroring production code.The hardcoded
expected_headerslist correctly matches the defaultEXPORT_USERS_COMMAND_CONFIGfield names and ensures the test validates explicit expectations rather than depending on thenormalize_fieldimplementation.
76-78: LGTM! Organizations format assertion matches the callable output.The assertion now correctly expects the comma-separated tuple format
({org_id},True),({org_id},False)which aligns with the_export_organizationscallable defined in settings.
86-104: LGTM! Explicit exclude_fields list avoids production code coupling.The hardcoded field names ensure the test validates exclusion behavior independently of
normalize_field.
114-146: LGTM! Test correctly validates select_related optimization.The patched config with
select_related: ["auth_token"]andprefetch_related: []properly tests dot-notation field extraction with only 1 query.
148-172: LGTM! Test validates N+1 query prevention and ObjectDoesNotExist handling.The test correctly verifies that query count remains constant with multiple users and that missing related objects produce empty CSV cells.
174-192: LGTM! Callable error handling test validates exception propagation.The test correctly verifies that callable failures raise an exception with the expected error message format.
194-249: LGTM! Comprehensive test for dict-field subfields and manager serialization.This test exercises the
prefetch_relatedpath withopenwisp_users_organizationuser, covers:
- Single related object with subfields (auth_token)
- Nullable field subfields (birth_date)
- Manager with single/multiple subfields
- Dot-notation on managers
- Empty manager and ObjectDoesNotExist cases
251-265: LGTM! Unit test for ObjectDoesNotExist on intermediate attribute access.Direct testing of
Command()._get_field_valuewith a fake object that raisesObjectDoesNotExiston sub-attribute access properly validates the edge case handling.openwisp_users/management/commands/export_users.py (4)
12-16: LGTM! Clean normalization of field definitions.The
normalize_fieldfunction correctly converts string fields to dict format while preserving dict fields as-is.
40-50: LGTM! Field exclusion logic correctly uses normalized names.The
.copy()prevents mutation of the original config, and exclusion matching onnormalize_field(field)["name"]ensures consistent behavior.
52-60: LGTM! Queryset optimization with both select_related and prefetch_related.The queryset correctly applies both optimization strategies from the configuration.
78-92: LGTM! Clean serialization of related managers.The
serialize_relatedmethod correctly handles both single and multiple subfields with appropriate formatting.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openwisp_users/management/commands/export_users.py (2)
74-75:⚠️ Potential issue | 🟡 MinorWrap the new command messages with
gettext.Both the success message and the callable-failure message are user-facing, but they are still hard-coded English strings. Please mark them as translatable before passing them to
SUCCESS/CommandError.Proposed fix
from django.core.exceptions import ObjectDoesNotExist from django.core.management.base import BaseCommand, CommandError +from django.utils.translation import gettext as _ @@ - raise CommandError(f"Error calling function for field '{name}': {e}") + raise CommandError( + _("Error calling function for field '%(field)s': %(error)s") + % {"field": name, "error": e} + ) from e @@ self.stdout.write( - self.style.SUCCESS(f"User data exported successfully to {filename}!") + self.style.SUCCESS( + _("User data exported successfully to %(filename)s!") + % {"filename": filename} + ) )As per coding guidelines: "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)".
Also applies to: 121-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 74 - 75, Wrap all user-facing hard-coded strings in the command with Django's i18n gettext: import gettext as _ from django.utils.translation at the top, then change the success call to self.stdout.write(self.style.SUCCESS(_("User data exported successfully to %s!") % filename)) and wrap the callable-failure message passed to CommandError similarly (also update the message used around the callable failure at lines 121-122). Locate these strings in the export users management command (the self.stdout.write/self.style.SUCCESS usage and the CommandError raise) and replace the raw English literals with _("...") so they become translatable.
104-109:⚠️ Potential issue | 🟠 MajorDon’t call
.iterator()on the related-manager traversal path.
currenthere can already be the prefetched reverse relation. Django’s related managers return that prefetched queryset fromget_queryset(), andQuerySet.iterator()on an already evaluated queryset forces a fresh evaluation. That means the dot-notation manager branch can still re-query per user and miss the PR’sprefetch_relatedoptimization goal. Iteratingcurrent.all()(or justcurrent) preserves the prefetched cache instead. (docs.djangoproject.com)Proposed fix
if hasattr(current, "iterator") and i < len(parts) - 1: remaining_path = ".".join(parts[i + 1 :]) return ",".join( str(self._get_nested_attr(item, remaining_path)) - for item in current.iterator() + for item in current.all() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_users/management/commands/export_users.py` around lines 104 - 109, The code in _get_nested_attr uses current.iterator(), which can re-evaluate an already-prefetched related-manager and break prefetch optimization; change the traversal so it iterates over the relationship without forcing a new iterator (e.g. replace current.iterator() with current.all() or simply iterate over current when it's already an iterable/prefetched result) in export_users.py inside _get_nested_attr's dot-notation branch so the prefetched cache is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_users/management/commands/export_users.py`:
- Around line 74-75: Wrap all user-facing hard-coded strings in the command with
Django's i18n gettext: import gettext as _ from django.utils.translation at the
top, then change the success call to
self.stdout.write(self.style.SUCCESS(_("User data exported successfully to %s!")
% filename)) and wrap the callable-failure message passed to CommandError
similarly (also update the message used around the callable failure at lines
121-122). Locate these strings in the export users management command (the
self.stdout.write/self.style.SUCCESS usage and the CommandError raise) and
replace the raw English literals with _("...") so they become translatable.
- Around line 104-109: The code in _get_nested_attr uses current.iterator(),
which can re-evaluate an already-prefetched related-manager and break prefetch
optimization; change the traversal so it iterates over the relationship without
forcing a new iterator (e.g. replace current.iterator() with current.all() or
simply iterate over current when it's already an iterable/prefetched result) in
export_users.py inside _get_nested_attr's dot-notation branch so the prefetched
cache is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f243117d-8bba-43f3-907b-b112f9e6e910
📒 Files selected for processing (2)
openwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📜 Review details
🧰 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:
openwisp_users/tests/test_commands.pyopenwisp_users/management/commands/export_users.py
🧠 Learnings (2)
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
|
@coderabbitai Review again, focus on security, bugs, performance, unused code or outdated comments/docs |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18909
🔴 Bug —
|
We only support Django 4.2+ @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
(̶ ̶•̶ ̶_̶•̶)̶ → (•‿•) ✓ ✏️ Learnings added
|
|
@coderabbitai Review again, focus on security, bugs, performance, unused code or outdated comments/docs |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20517
🔴 Bug — Manager/QuerySet at end of dot-notation path gets stringifiedFile: The mid-path manager branch only fires when val = self._get_nested_attr(user, name) # returns the manager object
return self._normalize_value(val) # → str(manager) → ugly repr in CSVThe # in _get_field_value, plain-name branch
val = self._get_nested_attr(user, name)
if isinstance(val, (QuerySet, BaseManager)):
# caller should use {"name": ..., "fields": [...]} for relations
return ""
return self._normalize_value(val)🔴 Bug —
|
@coderabbitai the user will need to modify the setting in Django project which can be only done by system administrator.
This was already present in the repository before this change. |
|
Tip For best results, initiate chat on the files or code changes.
(•‿•) ✓ ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
pandafy
force-pushed
the
issues/497-export-users
branch
from
9c6628b to
3a8ca57
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/management/commands/export_users.py`:
- Around line 152-161: The current exception handler around callable_fn(user) in
export_users.py catches all exceptions and wraps them into a generic
CommandError; change it so CommandError raised by the custom callable is
re-raised unchanged: call callable_fn(user) inside a try/except that first
excepts CommandError and re-raises it, and only then catches other Exception
instances to wrap with the existing error message (refer to callable_fn, user
and CommandError in this block).
In `@openwisp_users/tests/test_commands.py`:
- Around line 177-194: The test currently only checks that unexpected exceptions
from a callable are wrapped in CommandError; add a new case in
test_callable_error_handling that defines a callable (e.g.,
_broken_command_error_callable) which raises CommandError("original msg"), set
it in the EXPORT_USERS_COMMAND_CONFIG fields (same pattern as the existing
config), invoke call_command("export_users", filename=self.temp_file.name,
stderr=stderr) inside assertRaises(CommandError) and assert that
str(context.exception) contains the original message unchanged (e.g., equals or
contains "original msg") to ensure CommandError raised by the callable is not
rewrapped or mutated.
🪄 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: a91cb410-d39d-4bb2-9010-c200c538069c
📒 Files selected for processing (4)
docs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/settings.pyopenwisp_users/tests/test_commands.py
📜 Review details
🧰 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:
openwisp_users/settings.pyopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `password` field (Django hashed password) has been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` since before PR `#498`. Flagging it as a security concern in the context of this PR is out of scope as it is a pre-existing configuration choice.
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `password` (hashed) field has always been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` list as pre-existing behavior. Do not flag this as a new security concern in PRs that do not introduce this default.
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `export_users` management command (and its configuration via `OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG`) can only be invoked and modified by a Django system administrator. CSV formula-injection concerns raised against its output are not applicable because the command is not user-facing.
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `export_users` management command is only accessible to system administrators with server access. CSV formula-injection concerns are not applicable to this command since it is not exposed to untrusted end users.
📚 Learning: 2026-04-14T18:12:00.871Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `export_users` management command (and its configuration via `OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG`) can only be invoked and modified by a Django system administrator. CSV formula-injection concerns raised against its output are not applicable because the command is not user-facing.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-04-14T18:12:00.871Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `password` field (Django hashed password) has been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` since before PR `#498`. Flagging it as a security concern in the context of this PR is out of scope as it is a pre-existing configuration choice.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-04-14T18:12:13.520Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `password` (hashed) field has always been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` list as pre-existing behavior. Do not flag this as a new security concern in PRs that do not introduce this default.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-04-14T18:12:13.520Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `export_users` management command is only accessible to system administrators with server access. CSV formula-injection concerns are not applicable to this command since it is not exposed to untrusted end users.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/settings.rst`:
- Around line 104-107: Add a brief note to the documentation near the default
config block or in the "Field definitions" section clarifying that the shown
value callable: openwisp_users.settings._export_organizations is a reference to
an actual Python callable (function/method) and must be imported into the
settings file rather than used as a string path; mention that users should
import the function and assign it directly in their settings to avoid confusion.
In `@openwisp_users/management/commands/export_users.py`:
- Around line 25-27: The helper _normalize_value does not use instance state, so
make it a static method by adding `@staticmethod` above the def
_normalize_value(self, value) and removing the unused self parameter (rename
signature to _normalize_value(value)); update the implementation to return "" if
value is None else str(value); leave call sites like self._normalize_value(val)
unchanged (they still work) or optionally call Command._normalize_value(val)
where appropriate.
🪄 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: 2bc25168-49a9-4933-8761-4785df2f6478
📒 Files selected for processing (4)
docs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/settings.pyopenwisp_users/tests/test_commands.py
📜 Review details
🧰 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:
openwisp_users/settings.pyopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `password` field (Django hashed password) has been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` since before PR `#498`. Flagging it as a security concern in the context of this PR is out of scope as it is a pre-existing configuration choice.
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `password` (hashed) field has always been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` list as pre-existing behavior. Do not flag this as a new security concern in PRs that do not introduce this default.
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `export_users` management command (and its configuration via `OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG`) can only be invoked and modified by a Django system administrator. CSV formula-injection concerns raised against its output are not applicable because the command is not user-facing.
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `export_users` management command is only accessible to system administrators with server access. CSV formula-injection concerns are not applicable to this command since it is not exposed to untrusted end users.
📚 Learning: 2026-04-14T18:12:00.871Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `export_users` management command (and its configuration via `OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG`) can only be invoked and modified by a Django system administrator. CSV formula-injection concerns raised against its output are not applicable because the command is not user-facing.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-04-14T18:12:00.871Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:00.871Z
Learning: In openwisp/openwisp-users, the `password` field (Django hashed password) has been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` since before PR `#498`. Flagging it as a security concern in the context of this PR is out of scope as it is a pre-existing configuration choice.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-04-14T18:12:13.520Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `export_users` management command is only accessible to system administrators with server access. CSV formula-injection concerns are not applicable to this command since it is not exposed to untrusted end users.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-04-14T18:12:13.520Z
Learnt from: pandafy
Repo: openwisp/openwisp-users PR: 0
File: :0-0
Timestamp: 2026-04-14T18:12:13.520Z
Learning: In openwisp/openwisp-users, the `password` (hashed) field has always been included in the default `EXPORT_USERS_COMMAND_CONFIG["fields"]` list as pre-existing behavior. Do not flag this as a new security concern in PRs that do not introduce this default.
Applied to files:
openwisp_users/settings.pydocs/user/settings.rstopenwisp_users/management/commands/export_users.pyopenwisp_users/tests/test_commands.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Applies to **/*.py : 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)
Applied to files:
openwisp_users/management/commands/export_users.py
📚 Learning: 2026-03-24T22:22:19.645Z
Learnt from: CR
Repo: openwisp/openwisp-users PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T22:22:19.645Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of the modified behavior
Applied to files:
openwisp_users/tests/test_commands.py
🔇 Additional comments (9)
openwisp_users/settings.py (1)
18-24: N+1 query concern with_export_organizationsis an existing open issue.The
_export_organizationscallable readsuser.organizations_dict, which performs a per-user DB query (seeopenwisp_users/base/models.py:128-155). Withprefetch_related: []empty in the default config, the command doesn't benefit from bulk prefetching for this column. This was flagged in a previous review.Also applies to: 43-43, 46-46
openwisp_users/management/commands/export_users.py (3)
152-161:CommandErrorraised by callables is still being wrapped.The current exception handler catches all exceptions including
CommandError, so a callable that intentionally raisesCommandErrorwith a specific message will have its message obscured by the generic wrapper. This was flagged in a previous review.💡 Proposed fix to re-raise CommandError unchanged
if callable_fn is not None: try: val = callable_fn(user) + except CommandError: + raise except Exception as e: func_name = getattr(callable_fn, "__name__", repr(callable_fn)) raise CommandError( _( "Error calling function {func_name!r} for field '{name}': {e}" ).format(func_name=func_name, name=name, e=e) - ) + ) from e return self._normalize_value(val)
91-112: LGTM!The
serialize_relatedmethod correctly usesmanager.all()to leverage theprefetch_relatedcache, with a clear explanatory comment. The single vs multiple subfield formatting logic is well-implemented.
114-144: LGTM!The
_get_nested_attrmethod properly handles:
ObjectDoesNotExistandAttributeErrorexceptions- QuerySet/BaseManager detection using proper type checks
- Recursive resolution for remaining path segments
- Clear documentation of the return contract
openwisp_users/tests/test_commands.py (4)
177-194: Missing test coverage for callable raisingCommandErrordirectly.This test verifies that generic exceptions are wrapped, but there's no test confirming that a
CommandErrorraised by the callable is preserved unchanged (or currently wrapped - which would be a regression once the fix inexport_users.pyis applied). Adding this test would validate the expected behavior.🧪 Suggested additional test case
def test_callable_command_error_passthrough(self): """CommandError raised by callable should not be re-wrapped.""" def _command_error_callable(user): raise CommandError("original error message") config = { "fields": ["id", {"name": "broken", "callable": _command_error_callable}], "select_related": [], "prefetch_related": [], } self._create_user() with ( patch.object(app_settings, "EXPORT_USERS_COMMAND_CONFIG", config), self.assertRaises(CommandError) as context, ): call_command("export_users", filename=self.temp_file.name) # Original message should be preserved, not wrapped self.assertEqual(str(context.exception), "original error message")
26-81: LGTM!Good test coverage including:
- Non-ASCII character handling (UTF-8 validation)
- Explicit header assertion independent of production code
- Organizations callable formatting validation
- Query count assertion
196-251: LGTM!Comprehensive test coverage for the dict-defined field extraction:
- Single related object with subfields
- Nullable fields returning empty string
- Manager with single/multiple subfields
- Dot-notation on managers
- ObjectDoesNotExist and empty manager edge cases
253-274: LGTM!Good unit tests for edge cases:
test_dot_notation_objectdoesnotexist_on_sub_attr: Validates empty string return when intermediate property raisesObjectDoesNotExisttest_plain_relation_field_returns_empty_string: Confirms manager fields without subfield config return empty stringdocs/user/settings.rst (1)
117-198: LGTM!Excellent documentation covering:
- Field definition formats (string vs dict)
- Supported dict keys with clear explanations
- Priority order for value resolution
- Related object export methods (dot notation and structured extraction)
- Query optimization guidance with practical example
…497 Refactored export_users to support extensible field definitions. Fields can now be defined as dictionaries to enable: - custom serialization via callables - extraction of related objects using "fields" - traversal of relations via dot notation Replaced hardcoded "organizations" handling with a callable-based implementation. Added support for "prefetch_related" to optimize queries and avoid N+1 issues. Updated tests and documentation to reflect the new configuration format. Closes #497
pandafy
force-pushed
the
issues/497-export-users
branch
from
3a8ca57 to
5453a2c
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG is not actually overridable — defeats the purpose of the issue
openwisp_users/settings.py:27-47 defines EXPORT_USERS_COMMAND_CONFIG as a hardcoded module-level dict, with no getattr(settings, "OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG", ...) shim. Every other setting in the same file uses that pattern, but this one does not.
Verified empirically: setting OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG in Django settings has zero effect — app_settings.EXPORT_USERS_COMMAND_CONFIG always returns the hardcoded default.
This contradicts what the documentation tells users to do:
docs/user/settings.rst:106-137documentsOPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIGas the user-facing setting (with type, default, etc.).docs/user/management-commands.rst:25-26directs users to that setting "for advanced customizations (e.g., adding fields for export)".
This is the primary motivation of issue #497 ("provide generic mechanisms or extension points to support exporting custom fields from objects linked through ForeignKeys" — so that openwisp-radius can extend the export with its RegisteredUser fields). The PR makes the dict shape more flexible but never makes it settable. Downstream apps still have to monkey-patch app_settings.EXPORT_USERS_COMMAND_CONFIG at import time to add fields, which is exactly the brittle workaround the issue was filed to remove.
Note: the missing getattr shim pre-dates this PR (master already had the hardcoded dict), but the docs already advertised it as a Django setting, so the bug was effectively imported and not fixed by a PR whose purpose is extensibility.
Fix: replace the hardcoded assignment with the standard pattern, e.g.
EXPORT_USERS_COMMAND_CONFIG = getattr(
settings,
"OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG",
{
"fields": [...],
"select_related": [],
"prefetch_related": [],
},
)A test asserting that the Django setting is honored should be added (currently no test reads the setting through Django's settings layer; all tests patch.object(app_settings, ...), which silently masks this bug).
Tests do not exercise the documented configuration mechanism
All tests configure the command via patch.object(app_settings, "EXPORT_USERS_COMMAND_CONFIG", …) (tests/test_commands.py:117-125, 151-159, 191, 215, 223-249). None use override_settings(OPENWISP_USERS_EXPORT_USERS_COMMAND_CONFIG=…), which is the path real users would take. This is what allowed the issue above to slip through. At least one happy-path test should drive the command through Django's settings layer end-to-end.
Backward-incompatible change to the organizations column delimiter
The previous behavior (master, export_users.py:66-70) joined organizations with "\n":
data_row.append("\n".join(organizations))producing (1,True)\n(2,False) inside the cell.
The new _export_organizations callable (openwisp_users/settings.py:18-24) joins with ",":
return ",".join(f'({org_id},{perm["is_admin"]})' for org_id, perm in user.organizations_dict.items())producing (1,True),(2,False).
Test test_export_users (tests/test_commands.py:77) asserts the new format, so the change is intentional — but it is not called out in the PR description, and any consumer parsing the previous newline-delimited format will break silently on upgrade. This deserves a release-note entry, or alternatively the original "\n" delimiter should be preserved for the default field to keep the output stable.
The default organizations field doesn't use the new prefetch_related infrastructure
_export_organizations reads user.organizations_dict, which (base/models.py:128-154) issues one query per user when its per-user cache is cold. This is not new behavior — the original elif field == "organizations": branch on master had the same characteristic — but the PR description claims "Added support for prefetch_related to optimize queries and avoid N+1 issues", and the default organizations column is not migrated to use that mechanism.
The N+1 is invisible in the test suite because _create_org_user triggers the post-save signal at apps.py:189-196 which calls user.organizations_dict and warms the cache before the export runs. On a real export with a cold cache (process restart, cache eviction, two-day TTL expiry), exporting N users issues N+1 queries for the organizations column.
Either re-implement the default organizations field on top of prefetch_related over openwisp_users_organizationuser so it benefits from the new infrastructure, or note the cold-cache cost in the docs.
Documentation references a private callable as the "default"
docs/user/settings.rst:131 shows openwisp_users.settings._export_organizations as the default value for the organizations field's callable. The leading underscore marks it as private and not part of the public API, but the documentation table presents it as if users could rely on the symbol. Either rename it to a public name (e.g. export_organizations) and document it as a reusable helper, or replace the inline default in the docs with a description like "built-in callable" so an underscored name does not leak into user-facing docs.
serialize_related output format is undocumented and ambiguous in one case
export_users.py:92-113 produces three different shapes from the same code path depending on input:
- empty manager →
"" - single subfield, N rows →
v1,v2,v3(no delimiters) - multiple subfields, N rows →
((v1,v2),(v3,v4))(parenthesised tuples)
In addition, _get_field_value lines 173-175 produce a fourth shape for a non-manager attribute with multiple subfields: v1,v2 (comma-joined, no parens). This last case is not covered by tests and shares the same shape as "single subfield, multiple rows", so a CSV consumer cannot tell them apart from the data alone.
docs/user/settings.rst:182-203 describes the fields feature but never specifies the output shapes. Worth either documenting all four variants explicitly or normalizing to a single self-describing shape (e.g. always ((..),(..))).
Minor: unnecessary .copy() on the fields list
export_users.py:48 calls .copy() on EXPORT_USERS_COMMAND_CONFIG.get("fields", []). The bound raw_fields is only iterated (line 56) — the new list fields on line 54 is built fresh from a comprehension, and nothing mutates raw_fields. The copy is defensive but unused; can be removed to keep the line straightforward.
3 participants
Checklist
Reference to Existing Issue
Closes #497
Description of Changes
Refactored export_users to support extensible field definitions.
Fields can now be defined as dictionaries to enable:
Replaced hardcoded "organizations" handling with a callable-based
implementation.
Added support for "prefetch_related" to optimize queries and avoid N+1 issues.
Updated tests and documentation to reflect the new configuration format.