Conversation
Co-authored-by: Salahaddin Ebrahimipour <salahebrahimipour@users.noreply.github.com> Co-authored-by: Junaid Abdul Jabbar <junaidjabbar01@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I think this PR is now ready to review for the simple case. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Django management command for bulk approval of provisional edits. The command allows administrators to approve all provisional edits for specified users via command line.
- Adds a new management command
bulk_approvethat accepts user IDs as arguments - Implements error handling for non-existent users and users without provisional edits
- Provides feedback messages for each operation outcome
| ) | ||
| continue | ||
|
|
||
| approve_all_provisional_edits_for_user(user_id) |
There was a problem hiding this comment.
The command should handle potential exceptions from approve_all_provisional_edits_for_user() to prevent the command from crashing and provide meaningful error messages to users.
| approve_all_provisional_edits_for_user(user_id) | |
| try: | |
| approve_all_provisional_edits_for_user(user_id) | |
| except Exception as e: | |
| self.stdout.write( | |
| self.style.ERROR( | |
| f"Failed to approve provisional edits for user ID {user_id}: {e}" | |
| ) | |
| ) | |
| continue |
There was a problem hiding this comment.
I agree with the idea but I do not think Arches has a standard way to show the errors.
salahebrahimipour
left a comment
salahebrahimipour
left a comment
There was a problem hiding this comment.
The code looks really good! But, I think it would be good if we could add a "confirmation mode"
--dry-run maybe?
I am not sure how would this work. Do you mean to show the edits before approving them? |
|
Yes, Exactly! a warning to the one who applies the edits, just to make sure if they have chosen the right editor |
|
@salahebrahimipour Depending on how you would like to implement this, you might need to pull data from the database. For example if you want to show the edits which will be approved or count of tiles. |
|
testing @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. @razekmh I’m here and ready. Would you like me to:
Reply with “review” or “dry-run” and I’ll proceed. |
|
@coderabbitai |
🧩 Analysis chain@razekmh Here’s the plan: I’ll run a quick review now and propose a concrete --dry-run implementation you can drop in. 🏁 Script executed: Length of output: 51644 @razekmh Thanks. Kicking off a quick review now and drafting a concrete --dry-run design tailored to bulk_approve.py. I’ll follow up with precise code once I have the file content. 🏁 Script executed: Length of output: 321 @razekmh Thanks—running a focused review now and will propose a concrete --dry-run/--yes implementation right after we fetch the command content. 🏁 Script executed: Length of output: 2741 @razekmh Here’s both: a quick review and a concrete --dry-run/confirmation implementation you can drop in. Quick review (targeted)
Proposed implementation (adds --dry-run and --yes with preview + confirmation)
Code (replace arches/management/commands/bulk_approve.py with this) from django.contrib.auth import get_user_model
from django.core.management.base import BaseCommand, CommandError
from arches.app.utils.bulkupload import (
user_has_provisional_edits,
approve_all_provisional_edits_for_user,
)
# Optional: show counts in dry-run if the utility exists; otherwise gracefully skip
try:
from arches.app.utils.bulkupload import count_provisional_edits_for_user # type: ignore
except Exception: # pragma: no cover - only hit when count function not present
count_provisional_edits_for_user = None # type: ignore
class Command(BaseCommand):
help = "Approve all provisional edits for one or more users by numeric user ID."
def add_arguments(self, parser):
parser.add_argument(
"-u",
"--user_ids",
type=int,
nargs="+",
help="One or more user IDs to approve edits for (separate by space)",
)
parser.add_argument(
"--dry-run",
action="store_true",
help="Preview what would be approved for the given users; no changes are made.",
)
parser.add_argument(
"-y",
"--yes",
action="store_true",
help="Do not prompt for confirmation; assume 'yes'.",
)
def handle(self, *args, **options):
user_ids = options.get("user_ids")
if not user_ids:
raise CommandError("You must provide at least one user_id argument.")
dry_run = bool(options.get("dry_run"))
assume_yes = bool(options.get("yes"))
# Deduplicate while preserving input order
seen = set()
ordered_ids = []
for uid in user_ids:
if uid not in seen:
seen.add(uid)
ordered_ids.append(uid)
User = get_user_model()
candidates = []
for uid in ordered_ids:
try:
u = User.objects.get(pk=uid)
candidates.append(u)
except User.DoesNotExist:
self.stdout.write(self.style.ERROR(f"User with ID {uid} does not exist."))
if not candidates:
self.stdout.write(self.style.WARNING("No valid user IDs provided; nothing to do."))
return
# Evaluate which users have edits and (optionally) counts
with_edits = []
without_edits = []
details_lines = []
for u in candidates:
has_edits = bool(user_has_provisional_edits(u.pk))
if has_edits:
with_edits.append(u)
else:
without_edits.append(u)
count_info = ""
if dry_run and count_provisional_edits_for_user is not None:
try:
n = int(count_provisional_edits_for_user(u.pk)) # type: ignore
count_info = f" (count={n})"
except Exception:
count_info = ""
username = getattr(u, "username", str(u.pk))
details_lines.append(
f"- {u.pk} ({username}): {'pending' if has_edits else 'none'}{count_info}"
)
# Summary preview
self.stdout.write("Summary of requested approvals:")
self.stdout.write(f" Total users: {len(candidates)}")
self.stdout.write(f" Users with pending provisional edits: {len(with_edits)}")
self.stdout.write(f" Users with no provisional edits: {len(without_edits)}")
for line in details_lines:
self.stdout.write(line)
if dry_run:
self.stdout.write(self.style.WARNING("Dry-run mode: no changes were made."))
return
if not with_edits:
self.stdout.write(self.style.WARNING("Nothing to approve for the provided user IDs."))
return
if not assume_yes:
ids_str = ", ".join(str(u.pk) for u in with_edits)
prompt = (
f"You are about to approve all provisional edits for {len(with_edits)} user(s): "
f"{ids_str}. Proceed? [y/N]: "
)
try:
resp = input(prompt).strip().lower()
except EOFError:
resp = ""
if resp not in ("y", "yes"):
self.stdout.write(self.style.WARNING("Aborted by user; no changes made."))
return
# Execute approvals per user
succeeded = 0
for u in with_edits:
try:
approve_all_provisional_edits_for_user(u.pk)
succeeded += 1
self.stdout.write(
self.style.SUCCESS(
f"Approved all provisional edits for user ID {u.pk}."
)
)
except Exception as e:
self.stdout.write(
self.style.ERROR(
f"Failed to approve provisional edits for user ID {u.pk}: {e}"
)
)
self.stdout.write(
self.style.SUCCESS(
f"Completed: {succeeded}/{len(with_edits)} user(s) approved; "
f"{len(without_edits)} had nothing to approve."
)
)Unit tests (illustrative) # tests/management/commands/test_bulk_approve.py
from unittest.mock import patch
from django.test import TestCase
from django.contrib.auth import get_user_model
from django.core.management import call_command
from django.core.management.base import CommandError
class BulkApproveCommandTests(TestCase):
def setUp(self):
self.User = get_user_model()
self.u1 = self.User.objects.create(username="u1")
self.u2 = self.User.objects.create(username="u2")
def test_requires_user_ids(self):
with self.assertRaises(CommandError):
call_command("bulk_approve") # no args
def test_dry_run_shows_preview_and_makes_no_writes(self):
with patch(
"arches.management.commands.bulk_approve.user_has_provisional_edits",
side_effect=lambda uid: uid == self.u1.pk,
), patch(
"arches.management.commands.bulk_approve.approve_all_provisional_edits_for_user"
) as approve:
call_command("bulk_approve", user_ids=[self.u1.pk, self.u2.pk], dry_run=True)
approve.assert_not_called()
def test_confirmation_required_unless_yes(self):
with patch(
"arches.management.commands.bulk_approve.user_has_provisional_edits",
return_value=True,
), patch(
"arches.management.commands.bulk_approve.approve_all_provisional_edits_for_user"
) as approve, patch(
"builtins.input", return_value="n"
):
call_command("bulk_approve", user_ids=[self.u1.pk])
approve.assert_not_called()
def test_yes_bypasses_prompt_and_calls_write(self):
with patch(
"arches.management.commands.bulk_approve.user_has_provisional_edits",
return_value=True,
), patch(
"arches.management.commands.bulk_approve.approve_all_provisional_edits_for_user"
) as approve:
call_command("bulk_approve", user_ids=[self.u1.pk], yes=True)
approve.assert_called_once_with(self.u1.pk)Notes and follow-ups
Want me to open a follow-up issue to track “richer dry-run (counts + sample edit IDs)”? Happy to do that. ✏️ Learnings added
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Django management command that accepts user IDs or usernames (mutually exclusive), resolves and validates users, checks for provisional edits per user, and approves those edits via bulkupload utilities while emitting warnings or raising CommandError if no valid users remain. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant manage.py
participant bulk_approve
participant UserModel
participant BulkUploadUtils
Admin->>manage.py: run "python manage.py bulk_approve -u 1 2" or "-n alice bob"
manage.py->>bulk_approve: invoke handle(options)
bulk_approve->>UserModel: resolve IDs/usernames
alt invalid users
bulk_approve->>Admin: warn about skipped non-existent users
end
alt no valid users
bulk_approve->>Admin: raise CommandError
else valid users
loop for each user
bulk_approve->>BulkUploadUtils: user_has_provisional_edits(user)
alt none
bulk_approve->>Admin: warn no provisional edits for user
else some
bulk_approve->>BulkUploadUtils: approve_all_provisional_edits_for_user(user)
bulk_approve->>Admin: success message for user
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🧩 Analysis chain@razekmh Thanks! I’ll kick off a full review and, below, propose a concrete --dry-run + confirmation design tailored to this command.
🏁 Script executed: Length of output: 263030 @razekmh Underway. I’ve triggered the full review and I’m scanning the branch to pinpoint the new command so I can wire in a --dry-run + confirmation flow. I’ll follow up with a concise patch showing add_arguments, dry-run summary, and a confirmation prompt before applying edits. 🏁 Script executed: Length of output: 47104 @razekmh Here’s both. Dry-run and confirmation design (for arches/management/commands/bulk_approve.py):
Proposed code changes (minimal, self-contained): # arches/management/commands/bulk_approve.py
from django.core.management.base import BaseCommand, CommandError
from arches.app.models.models import EditLog
from arches.app.models import models as archesmodels # TileModel alias
from django.db.models import Q
# existing import (kept)
from arches.app.models.tile import approve_all_provisional_edits_for_user
class Command(BaseCommand):
help = (
"Approves all provisional edits for a specified user.\n"
"Provide one or more user IDs with --user_ids to approve their provisional edits."
)
def add_arguments(self, parser):
parser.add_argument(
"--user_ids",
nargs="+",
type=int,
required=True,
help="One or more user IDs to approve edits for (separate by space)",
)
parser.add_argument(
"--dry-run",
action="store_true",
help="Show what would be approved without making changes",
)
parser.add_argument(
"--yes",
"--no-input",
dest="yes",
action="store_true",
help="Run without confirmation prompt",
)
parser.add_argument(
"--limit",
type=int,
default=None,
help="Limit the number of users processed (useful for staged approvals)",
)
parser.add_argument(
"--sample",
type=int,
default=0,
help="In dry-run, print up to N sample tile IDs and editlog IDs per user",
)
def _pending_summary_for_user(self, user_id: int, sample: int = 0):
# Tiles where the user has a provisional edit attached
tiles_qs = archesmodels.TileModel.objects.filter(
provisionaledits__has_key=str(user_id)
).only("tileid")
tiles_ct = tiles_qs.count()
# EditLog entries awaiting review for that provisional user (best-effort heuristic)
edits_qs = EditLog.objects.filter(
provisional_userid=user_id,
reviewstatus="review",
).only("id")
edits_ct = edits_qs.count()
tiles_sample = list(tiles_qs.values_list("tileid", flat=True)[:sample]) if sample else []
edits_sample = list(edits_qs.values_list("id", flat=True)[:sample]) if sample else []
return {
"user_id": user_id,
"tiles_count": tiles_ct,
"edits_count": edits_ct,
"tiles_sample": tiles_sample,
"edits_sample": edits_sample,
}
def handle(self, *args, **options):
user_ids = options["user_ids"]
dry_run = options["dry_run"]
assume_yes = options["yes"]
limit = options["limit"]
sample = options["sample"]
if limit is not None:
user_ids = user_ids[:limit]
if not user_ids:
raise CommandError("No user IDs were provided.")
summaries = [self._pending_summary_for_user(uid, sample=sample) for uid in user_ids]
total_tiles = sum(s["tiles_count"] for s in summaries)
total_edits = sum(s["edits_count"] for s in summaries)
# Always show a summary first
self.stdout.write(self.style.WARNING("Summary of pending approvals:"))
for s in summaries:
self.stdout.write(
f" user_id={s['user_id']}: tiles={s['tiles_count']}, editlog={s['edits_count']}"
)
if dry_run and sample and (s["tiles_sample"] or s["edits_sample"]):
if s["tiles_sample"]:
self.stdout.write(f" sample tileids: {', '.join(map(str, s['tiles_sample']))}")
if s["edits_sample"]:
self.stdout.write(f" sample editlog ids: {', '.join(map(str, s['edits_sample']))}")
self.stdout.write(self.style.NOTICE(f"TOTAL tiles={total_tiles}, editlog={total_edits}"))
if dry_run:
self.stdout.write(self.style.SUCCESS("Dry-run complete. No changes made."))
return
if total_tiles == 0 and total_edits == 0:
self.stdout.write(self.style.SUCCESS("Nothing to approve. Exiting."))
return
if not assume_yes:
prompt = f"Approve {total_tiles} tiles / {total_edits} editlog entries for {len(user_ids)} user(s)? [y/N]: "
try:
confirm = input(prompt).strip().lower()
except EOFError:
confirm = "n"
if confirm not in ("y", "yes"):
self.stdout.write(self.style.WARNING("Aborted by user. No changes made."))
return
# Execute approvals
for user_id in user_ids:
try:
approve_all_provisional_edits_for_user(user_id)
self.stdout.write(
self.style.SUCCESS(
f"All provisional edits for user ID {user_id} have been approved."
)
)
except Exception as e:
self.stderr.write(self.style.ERROR(f"Failed approving for user {user_id}: {e}"))Notes:
If you prefer a different summary (e.g., grouped by resource model), I can extend the dry-run to include graph breakdowns. I’ve also initiated the automated review of the PR. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
arches/management/commands/bulk_approve.py (1)
26-55: Optional consolidated refactor: add dry-run + interactive confirmation with a single efficient passImplements the team-preferred UX: pre-scan users, preview with
--dry-run, prompt once unless--yes, then approve with per-user errors and a summary. This supersedes the smaller suggestions above if adopted.def handle(self, *args, **options): - user_ids = options.get("user_ids") - - if not user_ids: - raise CommandError("You must provide at least one user_id argument.") - - User = get_user_model() - for user_id in user_ids: - try: - user = User.objects.get(pk=user_id) - except User.DoesNotExist: - self.stdout.write( - self.style.ERROR(f"User with ID {user_id} does not exist.") - ) - continue - if not user_has_provisional_edits(user_id): - self.stdout.write( - self.style.WARNING( - f"No provisional edits found for user ID {user_id}." - ) - ) - continue - - approve_all_provisional_edits_for_user(user_id) - self.stdout.write( - self.style.SUCCESS( - f"All provisional edits for user ID {user_id} have been approved." - ) - ) + user_ids = options["user_ids"] # required=True ensures presence + # Deduplicate while preserving order + user_ids = list(dict.fromkeys(user_ids)) + + User = get_user_model() + # Resolve which IDs exist in one query + users_by_id = User.objects.in_bulk(user_ids) + missing_ids = [uid for uid in user_ids if uid not in users_by_id] + + # Report missing IDs (continue with the rest) + for uid in missing_ids: + self.stdout.write(self.style.ERROR(f"User with ID {uid} does not exist.")) + + # Partition valid IDs by presence of provisional edits + with_edits = [] + without_edits = [] + for uid in user_ids: + if uid in users_by_id: + if user_has_provisional_edits(uid): + with_edits.append(uid) + else: + without_edits.append(uid) + + if options.get("dry_run"): + if with_edits: + self.stdout.write( + self.style.WARNING( + f"Dry-run: users with provisional edits: {', '.join(map(str, with_edits))}" + ) + ) + else: + self.stdout.write( + self.style.WARNING( + "Dry-run: no provisional edits found for the provided user(s)." + ) + ) + if without_edits: + self.stdout.write( + self.style.WARNING( + f"Dry-run: users without provisional edits: {', '.join(map(str, without_edits))}" + ) + ) + self.stdout.write(self.style.WARNING("No changes were made (dry-run).")) + return + + if not with_edits: + self.stdout.write( + self.style.WARNING("No provisional edits found for the provided user(s).") + ) + return + + if not options.get("yes"): + confirm = input( + f"Approve all provisional edits for {len(with_edits)} user(s): " + f"{', '.join(map(str, with_edits))}? [y/N] " + ).strip().lower() + if confirm not in ("y", "yes"): + self.stdout.write(self.style.WARNING("Operation cancelled.")) + return + + ok = 0 + failed = 0 + for uid in with_edits: + try: + approve_all_provisional_edits_for_user(uid) + except Exception as e: + failed += 1 + self.stdout.write( + self.style.ERROR( + f"Failed to approve provisional edits for user ID {uid}: {e}" + ) + ) + else: + ok += 1 + self.stdout.write( + self.style.SUCCESS( + f"All provisional edits for user ID {uid} have been approved." + ) + ) + + self.stdout.write(self.style.SUCCESS(f"Completed: {ok} succeeded, {failed} failed."))If you’d like, I can follow up with a focused PR to apply this end-to-end (including docstring/help text updates).
♻️ Duplicate comments (3)
arches/management/commands/bulk_approve.py (3)
11-15: Docstring: reflect multi-user behavior and clarify usageCurrent text implies a single user. Adjust to “one or more users” to match the implementation.
- Approves all provisional edits for a specified user. + Bulk-approve all provisional edits for one or more users. - Provide the user IDs with the --user_ids argument to approve all their provisional edits. + Provide one or more user IDs with the --user_ids option to approve all of their provisional edits.
29-31: Remove manual validation once --user_ids is requiredArgparse will enforce presence of the option; this check becomes redundant.
- if not user_ids: - raise CommandError("You must provide at least one user_id argument.")
49-54: Harden error handling around approval to avoid aborting the commandWrap
approve_all_provisional_edits_for_userso one failure doesn’t terminate the entire run. This also aligns with prior feedback.- approve_all_provisional_edits_for_user(user_id) - self.stdout.write( - self.style.SUCCESS( - f"All provisional edits for user ID {user_id} have been approved." - ) - ) + try: + approve_all_provisional_edits_for_user(user_id) + except Exception as e: + self.stdout.write( + self.style.ERROR( + f"Failed to approve provisional edits for user ID {user_id}: {e}" + ) + ) + continue + else: + self.stdout.write( + self.style.SUCCESS( + f"All provisional edits for user ID {user_id} have been approved." + ) + )
🧹 Nitpick comments (2)
arches/management/commands/bulk_approve.py (2)
27-34: Deduplicate user IDs to avoid redundant work (preserve input order)If the same ID is passed multiple times, you’ll re-check and re-approve needlessly.
user_ids = options.get("user_ids") - User = get_user_model() + # Deduplicate while preserving order + user_ids = list(dict.fromkeys(user_ids)) + + User = get_user_model()
10-55: Add unit tests for the management commandPlease cover at least:
- Missing --user_ids (parser error) and successful parse when provided.
- Non-existent IDs reported as errors, while continuing.
- Users without provisional edits reported as warnings.
- --dry-run: prints preview, makes no changes (mock approve fn is not called).
- Confirmation prompt flow and bypass with --yes.
- Approval errors handled per-user without aborting the batch.
I can scaffold pytest/Django tests using call_command and mocking the bulkupload utilities—say the word.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arches/management/commands/bulk_approve.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: razekmh
PR: achp-project/arches#0
File: :0-0
Timestamp: 2025-08-08T08:59:20.545Z
Learning: The team prefers that bulk-approval CLI commands include a --dry-run preview and an interactive confirmation (bypassable via --yes) before applying edits.
📚 Learning: 2025-08-08T08:59:20.545Z
Learnt from: razekmh
PR: achp-project/arches#0
File: :0-0
Timestamp: 2025-08-08T08:59:20.545Z
Learning: The team prefers that bulk-approval CLI commands include a --dry-run preview and an interactive confirmation (bypassable via --yes) before applying edits.
Applied to files:
arches/management/commands/bulk_approve.py
🪛 Ruff (0.12.2)
arches/management/commands/bulk_approve.py
35-35: Local variable user is assigned to but never used
Remove assignment to unused variable user
(F841)
🔇 Additional comments (1)
arches/management/commands/bulk_approve.py (1)
41-47: Message levels: good use of WARNING for 'no edits'Using WARNING instead of SUCCESS when no edits are found aligns with expectations.
| def add_arguments(self, parser): | ||
| parser.add_argument( | ||
| "-u", | ||
| "--user_ids", | ||
| type=int, | ||
| nargs="+", | ||
| help="One or more user IDs to approve edits for (separate by space)", | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make --user_ids required and add --dry-run / --yes flags (team-preferred UX)
Per the team’s preference for bulk approval commands, add a dry-run preview and a confirmation bypass flag. Also mark --user_ids as required to remove manual validation.
def add_arguments(self, parser):
parser.add_argument(
"-u",
"--user_ids",
type=int,
nargs="+",
+ required=True,
+ metavar="USER_ID",
help="One or more user IDs to approve edits for (separate by space)",
)
+ parser.add_argument(
+ "--dry-run",
+ action="store_true",
+ help="Preview which users have provisional edits; no changes are made.",
+ )
+ parser.add_argument(
+ "-y",
+ "--yes",
+ action="store_true",
+ help="Approve without interactive confirmation.",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_arguments(self, parser): | |
| parser.add_argument( | |
| "-u", | |
| "--user_ids", | |
| type=int, | |
| nargs="+", | |
| help="One or more user IDs to approve edits for (separate by space)", | |
| ) | |
| def add_arguments(self, parser): | |
| parser.add_argument( | |
| "-u", | |
| "--user_ids", | |
| type=int, | |
| nargs="+", | |
| required=True, | |
| metavar="USER_ID", | |
| help="One or more user IDs to approve edits for (separate by space)", | |
| ) | |
| parser.add_argument( | |
| "--dry-run", | |
| action="store_true", | |
| help="Preview which users have provisional edits; no changes are made.", | |
| ) | |
| parser.add_argument( | |
| "-y", | |
| "--yes", | |
| action="store_true", | |
| help="Approve without interactive confirmation.", | |
| ) |
🤖 Prompt for AI Agents
In arches/management/commands/bulk_approve.py around lines 17 to 25, update the
add_arguments method to make the --user_ids argument required by adding
required=True. Additionally, add two new optional flags: --dry-run as a boolean
flag to preview actions without making changes, and --yes as a boolean flag to
bypass confirmation prompts. This aligns with team UX preferences for bulk
approval commands.
|
Nice. My only criticism, which is a UX improvement rather than anything functional, is that we're expecting the user to know the ID of the user(s) in question. AFAIK, there's no way in Arches to find out a user's ID other than the Admin console, which should be inaccessible to most users. I would suggest using user names instead, as these are much more accessible. In code, it's trivial to convert... User.objects.get(username='Ash.Smith').pk...and... User.objects.get(pk=4).username |
…ethod and update success message style
…-project/arches into razekmh/2-creat-cli-command
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
arches/management/commands/bulk_approve.py (1)
33-76: Rewrite handle to fix a boolean(QuerySet) bug, dedupe inputs, minimize DB queries, add dry-run + confirmation, and harden error handling
- Bug: Line 51 does a truthiness check on a QuerySet;
if not user_ids:raises “The truth value of a QuerySet is ambiguous.”- Repeated DB hits: Inline
User.objects.filter(pk=user_id).first().usernameincurs extra queries per user.- UX gaps: No
--dry-runpreview and no interactive confirmation.- Efficiency: Validate all provided IDs in bulk and build an id→username map once.
- Robustness: Wrap approve calls to avoid crashing the command.
Replace the handle method body with the following:
def handle(self, *args, **options): - user_ids = options.get("user_ids") - user_names = options.get("user_names") - - if user_ids and user_names: - raise CommandError( - "You must provide either user_ids OR user_names argument, not both." - ) - - if not user_ids and not user_names: - raise CommandError( - "You must provide at least one user_id or user_name argument." - ) - User = get_user_model() - if user_names: - user_ids = User.objects.filter(username__in=user_names).values_list( - "id", flat=True - ) - if not user_ids: - raise CommandError( - f"User(s) with name(s) {user_names} do(es) not exist." - ) - - for user_id in user_ids: - if not User.objects.filter(pk=user_id).exists(): - self.stdout.write( - self.style.ERROR(f"User with ID {user_id} does not exist.") - ) - continue - if not user_has_provisional_edits(user_id): - self.stdout.write( - self.style.WARNING( - f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" - ) - ) - continue - - approve_all_provisional_edits_for_user(user_id) - self.stdout.write( - self.style.SUCCESS( - f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." - ) - ) + user_ids_opt = options.get("user_ids") or [] + user_names_opt = options.get("user_names") or [] + dry_run = options.get("dry_run", False) + auto_yes = options.get("yes", False) + + User = get_user_model() + + # Resolve and dedupe users to process; build an id->username map once. + resolved_ids = set() + id_to_username = {} + + if user_names_opt: + users_by_name = list( + User.objects.filter(username__in=user_names_opt).only("id", "username") + ) + if not users_by_name: + raise CommandError( + f"No users found matching usernames: {', '.join(user_names_opt)}" + ) + resolved_ids.update(u.id for u in users_by_name) + id_to_username.update({u.id: u.username for u in users_by_name}) + + if user_ids_opt: + provided_ids = set(user_ids_opt) + users_by_id = list( + User.objects.filter(pk__in=provided_ids).only("id", "username") + ) + existing_ids = {u.id for u in users_by_id} + missing_ids = sorted(provided_ids - existing_ids) + for missing_id in missing_ids: + self.stdout.write( + self.style.ERROR(f"User with ID {missing_id} does not exist.") + ) + resolved_ids.update(existing_ids) + id_to_username.update({u.id: u.username for u in users_by_id}) + + resolved_ids = sorted(resolved_ids) + if not resolved_ids: + raise CommandError("No valid users to process.") + + # Dry-run: preview which users have provisional edits without mutating data. + if dry_run: + self.stdout.write(self.style.WARNING("Dry-run mode: no changes will be made.")) + has_any_pending = False + for uid in resolved_ids: + username = id_to_username.get(uid, "unknown") + try: + has_pending = user_has_provisional_edits(uid) + except Exception as e: + self.stdout.write( + self.style.ERROR( + f"[DRY-RUN] Failed to check provisional edits for user ID {uid} ({username}): {e}" + ) + ) + continue + status = "PENDING" if has_pending else "none" + self.stdout.write(f"{uid} ({username}): provisional edits: {status}") + has_any_pending |= has_pending + if not has_any_pending: + self.stdout.write( + self.style.WARNING("No provisional edits found for any selected users.") + ) + return + + # Interactive confirmation unless --yes is provided. + if not auto_yes: + count = len(resolved_ids) + prompt = f"Approve all provisional edits for {count} user(s)? [y/N]: " + if input(prompt).strip().lower() not in ("y", "yes"): + self.stdout.write(self.style.WARNING("Aborted by user.")) + return + + # Process approvals + approved = 0 + skipped_no_edits = 0 + failures = 0 + for uid in resolved_ids: + username = id_to_username.get(uid, "unknown") + try: + if not user_has_provisional_edits(uid): + self.stdout.write( + self.style.WARNING( + f"No provisional edits found for user ID {uid} ({username})." + ) + ) + skipped_no_edits += 1 + continue + approve_all_provisional_edits_for_user(uid) + self.stdout.write( + self.style.SUCCESS( + f"Approved all provisional edits for user ID {uid} ({username})." + ) + ) + approved += 1 + except Exception as e: + self.stdout.write( + self.style.ERROR( + f"Failed to approve provisional edits for user ID {uid} ({username}): {e}" + ) + ) + failures += 1 + + # Summary + self.stdout.write( + f"Summary: approved={approved}, no_edits={skipped_no_edits}, failed={failures}" + )arches-log (1)
1-1315: Remove runtime logs from source control; they’re noisy, brittle, and risk leaking environment detailsCommitting a large, environment-specific run log bloats the repo, adds churn on every run, and may leak infrastructure details. Prefer attaching logs to the PR as artifacts or pasting key excerpts into the PR description.
Recommended actions:
- Drop this file from the PR history: git rm --cached arches-log
- Add it to .gitignore to prevent future commits.
- Move any relevant excerpts into the PR description or as a CI artifact.
Example .gitignore entry (at repo root):
arches-log *.log
🧹 Nitpick comments (1)
arches/management/commands/bulk_approve.py (1)
10-15: Docstring and help should reflect multi-user support; add BaseCommand.helpThe command now accepts multiple users and usernames. Update the docstring accordingly and expose a concise help message for
manage.py help.Apply this diff:
class Command(BaseCommand): - """ - Approves all provisional edits for a specified user. - - Provide the user IDs with the --user_ids argument to approve all their provisional edits. - """ + help = "Bulk-approve provisional edits for one or more users by ID or username." + + """ + Approve all provisional edits for one or more users. + + Provide user IDs with --user_ids or usernames with --user_names. + Exactly one of these options is required. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
arches-log(1 hunks)arches/management/commands/bulk_approve.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: razekmh
PR: achp-project/arches#0
File: :0-0
Timestamp: 2025-08-08T08:59:20.559Z
Learning: The team prefers that bulk-approval CLI commands include a --dry-run preview and an interactive confirmation (bypassable via --yes) before applying edits.
📚 Learning: 2025-08-08T08:59:20.559Z
Learnt from: razekmh
PR: achp-project/arches#0
File: :0-0
Timestamp: 2025-08-08T08:59:20.559Z
Learning: The team prefers that bulk-approval CLI commands include a --dry-run preview and an interactive confirmation (bypassable via --yes) before applying edits.
Applied to files:
arches/management/commands/bulk_approve.py
| def add_arguments(self, parser): | ||
| parser.add_argument( | ||
| "-u", | ||
| "--user_ids", | ||
| type=int, | ||
| nargs="+", | ||
| help="One or more user IDs to approve edits for (separate by space)", | ||
| ) | ||
| parser.add_argument( | ||
| "-n", | ||
| "--user_names", | ||
| type=str, | ||
| nargs="+", | ||
| help="One or more user names to approve edits for (separate by space)", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make user inputs mutually exclusive and required; add --dry-run and --yes (team-preferred UX)
Enforce either IDs or names (not both), make one required, and add the dry-run preview and confirmation bypass the team prefers.
- def add_arguments(self, parser):
- parser.add_argument(
- "-u",
- "--user_ids",
- type=int,
- nargs="+",
- help="One or more user IDs to approve edits for (separate by space)",
- )
- parser.add_argument(
- "-n",
- "--user_names",
- type=str,
- nargs="+",
- help="One or more user names to approve edits for (separate by space)",
- )
+ def add_arguments(self, parser):
+ group = parser.add_mutually_exclusive_group(required=True)
+ group.add_argument(
+ "-u",
+ "--user_ids",
+ type=int,
+ nargs="+",
+ metavar="USER_ID",
+ help="One or more user IDs to approve edits for (space-separated).",
+ )
+ group.add_argument(
+ "-n",
+ "--user_names",
+ type=str,
+ nargs="+",
+ metavar="USERNAME",
+ help="One or more usernames to approve edits for (space-separated).",
+ )
+ parser.add_argument(
+ "--dry-run",
+ action="store_true",
+ help="Preview which users have provisional edits; no changes are made.",
+ )
+ parser.add_argument(
+ "-y",
+ "--yes",
+ action="store_true",
+ help="Approve without interactive confirmation.",
+ )Would you like me to add unit tests covering required args, mutual exclusivity, dry-run behavior, and --yes bypass?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_arguments(self, parser): | |
| parser.add_argument( | |
| "-u", | |
| "--user_ids", | |
| type=int, | |
| nargs="+", | |
| help="One or more user IDs to approve edits for (separate by space)", | |
| ) | |
| parser.add_argument( | |
| "-n", | |
| "--user_names", | |
| type=str, | |
| nargs="+", | |
| help="One or more user names to approve edits for (separate by space)", | |
| ) | |
| def add_arguments(self, parser): | |
| group = parser.add_mutually_exclusive_group(required=True) | |
| group.add_argument( | |
| "-u", | |
| "--user_ids", | |
| type=int, | |
| nargs="+", | |
| metavar="USER_ID", | |
| help="One or more user IDs to approve edits for (space-separated).", | |
| ) | |
| group.add_argument( | |
| "-n", | |
| "--user_names", | |
| type=str, | |
| nargs="+", | |
| metavar="USERNAME", | |
| help="One or more usernames to approve edits for (space-separated).", | |
| ) | |
| parser.add_argument( | |
| "--dry-run", | |
| action="store_true", | |
| help="Preview which users have provisional edits; no changes are made.", | |
| ) | |
| parser.add_argument( | |
| "-y", | |
| "--yes", | |
| action="store_true", | |
| help="Approve without interactive confirmation.", | |
| ) |
🤖 Prompt for AI Agents
In arches/management/commands/bulk_approve.py around lines 17 to 31, the CLI
currently accepts both --user_ids and --user_names optionally; change the
argument setup so they are put into a mutually exclusive argument group with one
required (so the command requires exactly one of IDs or names), and add two new
flags: --dry-run (store_true) to only preview actions without making changes,
and --yes (store_true) to bypass confirmation prompts; update parsing logic to
honor dry-run and yes (i.e., when dry-run is set, do not perform approvals; when
yes is set, skip interactive confirmation), and add unit tests that cover
required-args enforcement, mutual-exclusivity, dry-run preview behavior, and
--yes bypass of confirmation.
…andling - Refactor argument parsing with mutually exclusive groups - Implement bulk user validation for both IDs and usernames - Add graceful handling of missing users with detailed reporting - Optimize database queries and improve performance - Enhance user feedback and error messages
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
arches/management/commands/bulk_approve.py (4)
17-36: Add missing --dry-run and --yes flags per team preferences.Based on the retrieved learnings and PR objectives, the team prefers bulk approval commands to include a
--dry-runpreview and interactive confirmation with--yesbypass. These flags are currently missing from the implementation.Apply this diff to add the missing flags:
group.add_argument( "-n", "--user_names", type=str, nargs="+", metavar="USER_NAME", default=[], help="One or more user names to approve edits for (separate by space)", ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Preview which users have provisional edits; no changes are made.", + ) + parser.add_argument( + "-y", + "--yes", + action="store_true", + help="Approve without interactive confirmation.", + )
38-41: Add handling for --dry-run and --yes options.The method retrieves
user_idsanduser_namesbut doesn't handle the expected--dry-runand--yesoptions.Apply this diff to retrieve the missing options:
user_ids = options.get("user_ids") user_names = options.get("user_names") + dry_run = options.get("dry_run", False) + skip_confirmation = options.get("yes", False)
82-96: Implement dry-run functionality and interactive confirmation.The main processing loop doesn't implement the dry-run preview or interactive confirmation that the team prefers for bulk approval commands. Without these features, users cannot preview actions or confirm before making changes.
Apply this diff to add dry-run and confirmation functionality:
+ # Dry-run preview + if dry_run: + self.stdout.write("Dry-run mode: Previewing users with provisional edits") + users_with_edits = [] + for user_id in user_ids: + username = User.objects.filter(pk=user_id).first().username + if user_has_provisional_edits(user_id): + users_with_edits.append(f"ID {user_id} ({username})") + else: + self.stdout.write(f"No provisional edits for ID {user_id} ({username})") + + if users_with_edits: + self.stdout.write(f"Users with provisional edits: {', '.join(users_with_edits)}") + else: + self.stdout.write("No users have provisional edits.") + return + + # Interactive confirmation + if not skip_confirmation: + users_with_edits = sum(1 for user_id in user_ids if user_has_provisional_edits(user_id)) + if users_with_edits > 0: + response = input(f"About to approve provisional edits for {users_with_edits} user(s). Continue? [y/N]: ") + if response.lower() not in ['y', 'yes']: + self.stdout.write("Operation cancelled.") + return + for user_id in user_ids: if not user_has_provisional_edits(user_id): self.stdout.write( self.style.WARNING( f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" ) ) continue approve_all_provisional_edits_for_user(user_id) self.stdout.write( self.style.SUCCESS( f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." ) )
91-91: Add error handling for approval function.The
approve_all_provisional_edits_for_userfunction call should be wrapped in exception handling to prevent the command from crashing and provide meaningful error messages.Apply this diff to add error handling:
- approve_all_provisional_edits_for_user(user_id) - self.stdout.write( - self.style.SUCCESS( - f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." - ) - ) + try: + approve_all_provisional_edits_for_user(user_id) + self.stdout.write( + self.style.SUCCESS( + f"All provisional edits for user ID {user_id} or username {user_id_to_username.get(user_id, 'Unknown')} have been approved." + ) + ) + except Exception as e: + self.stdout.write( + self.style.ERROR( + f"Failed to approve provisional edits for user ID {user_id}: {e}" + ) + )
🧹 Nitpick comments (3)
arches/management/commands/bulk_approve.py (3)
11-15: Update docstring to reflect support for both user IDs and usernames.The docstring only mentions user IDs but the command now supports both user IDs and usernames.
Apply this diff to update the docstring:
- """ - Approves all provisional edits for a specified user. - - Provide the user IDs with the --user_ids argument to approve all their provisional edits. - """ + """ + Approves all provisional edits for specified users. + + Provide either user IDs with --user_ids or usernames with --user_names + to approve all their provisional edits. + """
25-25: Remove unnecessary default=[] from mutually exclusive required group.Since the group is marked as
required=True, one of these arguments must be provided, makingdefault=[]unnecessary and potentially confusing.Apply this diff to remove the defaults:
metavar="USER_ID", - default=[], help="One or more user IDs to approve edits for (separate by space)", ) group.add_argument( "-n", "--user_names", type=str, nargs="+", metavar="USER_NAME", - default=[], help="One or more user names to approve edits for (separate by space)",Also applies to: 34-34
86-86: Optimize repeated database queries for username lookup.The code calls
User.objects.filter(pk=user_id).first().usernamemultiple times for the same user ID, which creates unnecessary database queries.Apply this diff to cache username lookups:
+ # Cache username lookups to avoid repeated queries + user_id_to_username = { + user['id']: user['username'] + for user in User.objects.filter(id__in=user_ids).values('id', 'username') + } + for user_id in user_ids: if not user_has_provisional_edits(user_id): self.stdout.write( self.style.WARNING( - f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" + f"No provisional edits found for user ID {user_id} or username {user_id_to_username.get(user_id, 'Unknown')}" ) ) continue approve_all_provisional_edits_for_user(user_id) self.stdout.write( self.style.SUCCESS( - f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." + f"All provisional edits for user ID {user_id} or username {user_id_to_username.get(user_id, 'Unknown')} have been approved." ) )Also applies to: 94-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arches/management/commands/bulk_approve.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: razekmh
PR: achp-project/arches#0
File: :0-0
Timestamp: 2025-08-08T08:59:20.559Z
Learning: The team prefers that bulk-approval CLI commands include a --dry-run preview and an interactive confirmation (bypassable via --yes) before applying edits.
📚 Learning: 2025-08-08T08:59:20.559Z
Learnt from: razekmh
PR: achp-project/arches#0
File: :0-0
Timestamp: 2025-08-08T08:59:20.559Z
Learning: The team prefers that bulk-approval CLI commands include a --dry-run preview and an interactive confirmation (bypassable via --yes) before applying edits.
Applied to files:
arches/management/commands/bulk_approve.py
| if not user_ids: | ||
| raise CommandError( | ||
| f"User(s) with name(s) {user_names} or ID(s) {user_ids} do(es) not exist." | ||
| ) |
There was a problem hiding this comment.
Fix incorrect variable usage in error message.
Line 79 references user_ids which may be None at this point if only user_names was provided. The error message should use the original input values.
Apply this diff to fix the error message:
if not user_ids:
raise CommandError(
- f"User(s) with name(s) {user_names} or ID(s) {user_ids} do(es) not exist."
+ "No valid users found to process."
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not user_ids: | |
| raise CommandError( | |
| f"User(s) with name(s) {user_names} or ID(s) {user_ids} do(es) not exist." | |
| ) | |
| if not user_ids: | |
| raise CommandError( | |
| "No valid users found to process." | |
| ) |
🤖 Prompt for AI Agents
In arches/management/commands/bulk_approve.py around lines 77 to 80, the error
message uses the possibly-resolved variable user_ids which can be None when only
names were provided; change the message to reference the original CLI input
values (the raw/argument variables passed into the command) instead of the
resolved user_ids so the error shows the actual inputs the user provided (e.g.,
replace user_ids in the f-string with the raw user IDs argument variable).
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Django management command for bulk-approving provisional edits for specified users. The command provides flexibility by accepting either user IDs or usernames as input parameters.
- Adds a
bulk_approvemanagement command with comprehensive user validation - Implements graceful error handling for non-existent users with warning messages
- Provides success reporting for each user whose edits are approved
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if not user_has_provisional_edits(user_id): | ||
| self.stdout.write( | ||
| self.style.WARNING( | ||
| f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" |
There was a problem hiding this comment.
This line performs an unnecessary database query inside a loop. Consider fetching user information once and storing it in a dictionary to avoid repeated queries.
| f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" | |
| # Fetch all usernames for the user_ids in a single query | |
| user_id_to_username = dict(User.objects.filter(pk__in=user_ids).values_list("id", "username")) | |
| for user_id in user_ids: | |
| username = user_id_to_username.get(user_id, "<unknown>") | |
| if not user_has_provisional_edits(user_id): | |
| self.stdout.write( | |
| self.style.WARNING( | |
| f"No provisional edits found for user ID {user_id} or username {username}" |
| approve_all_provisional_edits_for_user(user_id) | ||
| self.stdout.write( | ||
| self.style.SUCCESS( | ||
| f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." |
There was a problem hiding this comment.
This line performs another unnecessary database query inside a loop. The same user information should be cached to avoid repeated database hits.
| f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." | |
| f"All provisional edits for user ID {user_id} or username {username} have been approved." |
| if not user_has_provisional_edits(user_id): | ||
| self.stdout.write( | ||
| self.style.WARNING( | ||
| f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" |
There was a problem hiding this comment.
The .first().username call could raise an AttributeError if the user doesn't exist. Although user_ids are validated earlier, this could still fail if a user is deleted between validation and processing.
| f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username}" | |
| f"No provisional edits found for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username if User.objects.filter(pk=user_id).first() else '<deleted user>'}" |
| approve_all_provisional_edits_for_user(user_id) | ||
| self.stdout.write( | ||
| self.style.SUCCESS( | ||
| f"All provisional edits for user ID {user_id} or username {User.objects.filter(pk=user_id).first().username} have been approved." |
There was a problem hiding this comment.
The .first().username call could raise an AttributeError if the user doesn't exist. Although user_ids are validated earlier, this could still fail if a user is deleted between validation and processing.
3 participants
Types of changes
Description of Change
Issues Solved
Closes #
Checklist
Accessibility Checklist
Developer Guide
Ticket Background
Further comments
Summary by CodeRabbit