Skip to content

Conversation

@snukky
Copy link
Contributor

@snukky snukky commented Aug 11, 2025

This PR adds an option to do ExportSystemScoresToCSV --reset-accounts-only to export scores from the reset accounts.

@AppraiseDev/core-team

@zouharvi
Copy link
Contributor

  • The --reset-account-only seems to imply that by default it's completed+reset items but that's not true. So maybe rename this flag to --include-reset-accounts? This is different from --completed-only which makes a subset of all accounts while --reset-account-only seems to widen it and operate on segment and not account level?
  • Internally it seems that the name for this is retired, so perhaps the flag name should be --include-retired?

@snukky
Copy link
Contributor Author

snukky commented Aug 11, 2025

At the moment, this option, if turned on, outputs only the accounts from reset accounts and ignores --completed-only parameter, i.e., it doesn't "append" to the scores. I'm not sure if that was clear, so mentioning.

If I understand correctly, you'd prefer this to add extra scores to scores from active accounts? If so, I agree that a name like --include-reset-accounts will be better.

As for retired vs reset-accounts, I'd slightly prefer the latter as retired seems more specific to the code base and I'm not sure if this is the best description for UI. Let me know what you think.

@zouharvi
Copy link
Contributor

I see, thanks for the clarification. In this context, --reset-accounts-only makes sense. Maybe add:

assert not (include_retired and completed_only), "--include-reset-accounts and --completed-only are mutually exclusive"

@zouharvi
Copy link
Contributor

Could you also pass include_retired=True by default here in the download action?

csv_content = cls.get_system_data(current_campaign.id, extended_csv=True)

@zouharvi
Copy link
Contributor

zouharvi commented Aug 11, 2025

I still don't understand how this line works:

        if include_retired:
            qs = cls.objects.filter(retired=True, item__itemType__in=item_types)

Won't it select only retired items? And does it imply completed=True? (on item-level)

@snukky
Copy link
Contributor Author

snukky commented Aug 12, 2025

I still don't understand how this line works:

        if include_retired:
            qs = cls.objects.filter(retired=True, item__itemType__in=item_types)

Won't it select only retired items? And does it imply completed=True? (on item-level)

Yes, it will select only retired items. It's a bad name for this param. Now I see where the confusion comes from. Thanks for noticing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants