Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic to migrate fieldreport number #2349

Draft
wants to merge 8 commits into
base: project/field-report-translations
Choose a base branch
from

Conversation

sudip-khanal
Copy link

Addresses

Migration of Field Report Data for Dynamic Title Suffixes

Changes

  • Add logic to migrate field report number from the old summary

Checklist

Things that should succeed before merging.

  • Updated/ran unit tests
  • Updated CHANGELOG.md

Release

If there is a version update, make sure to tag the repository with the latest version.

Copy link
Member

@susilnem susilnem left a comment

Choose a reason for hiding this comment

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

Minor changes,
let's change the file name to migrate_field_report_number

api/management/commands/field-report-fix-suffixes.py Outdated Show resolved Hide resolved
api/management/commands/field-report-fix-suffixes.py Outdated Show resolved Hide resolved
Copy link
Member

@susilnem susilnem left a comment

Choose a reason for hiding this comment

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

Few changes

@@ -43,7 +42,8 @@ def handle(self, *args, **kwargs):

# Update the group data if this report has the highest fr number
if max_fr_num > group_data["highest_fr_num"]:
self.stdout.write(f"Updating highest fr_num for group (event_id={report.event.id}, country_id={country.id})")

print(f"Updating highest fr_num for group (event_id={report.event.id}, country_id={country.id})")
Copy link
Member

Choose a reason for hiding this comment

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

Remove Printf

@@ -53,12 +53,18 @@ def handle(self, *args, **kwargs):
highest_fr_num = data["highest_fr_num"]

if highest_report:

print(f"Setting fr_num={highest_fr_num} for report ID={highest_report.id}")
Copy link
Member

Choose a reason for hiding this comment

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

here

self.stdout.write("Completed successfully.")
excluded_reports.update(fr_num=None)

print(f"Set fr_num=None for reports in group (event_id={event_id}, country_id={country_id})")
Copy link
Member

Choose a reason for hiding this comment

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

Here

Comment on lines 62 to 63
excluded_reports = FieldReport.objects.filter(event_id=event_id, countries=country_id).exclude(
id=highest_report.id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
excluded_reports = FieldReport.objects.filter(event_id=event_id, countries=country_id).exclude(
id=highest_report.id
FieldReport.objects.filter(event_id=event_id, countries=country_id).exclude(
id=highest_report.id).update(fr_num=None)

Comment on lines 59 to 62
# Set fr_num to null for all other reports in the group
FieldReport.objects.filter(event_id=event_id, countries=country_id).exclude(id=highest_report.id).update(
fr_num=None
)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? We shouldn't mutate existing data if not required.

if highest_report:

highest_report.fr_num = highest_fr_num
highest_report.save(update_fields=["fr_num"])
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 34 to 40
if key not in event_country_data:
event_country_data[key] = {
"highest_fr_num": 0,
"report_highest_fr": None,
}

group_data = event_country_data[key]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if key not in event_country_data:
event_country_data[key] = {
"highest_fr_num": 0,
"report_highest_fr": None,
}
group_data = event_country_data[key]
group_data = event_country_data[key] = event_country_data.get(key, {
"highest_fr_num": 0,
"report_highest_fr": None,
})

def handle(self, *args, **kwargs):
suffix_pattern = re.compile(r"#(\d+)")

reports = FieldReport.objects.filter(summary__icontains="#", event__isnull=False, countries__isnull=False)
Copy link
Member

Choose a reason for hiding this comment

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

Let's exclude any event/country field reports with fr_num field with value.

Something like this

# Only process event/country where we don't have fr_num defined
event_country_filter_qs = Report.objects.order_by().values('event', 'countries').annoate(
    count=models.Count('id', filter=models.Q(fr_num__isnull=True))
).exclude(count__gte=1)

report_to_process_qs = Report.objects.filter(
    # Still slow but better then doing this within python
    event__in=event_country_filter_qs.values('event'),
    countries__in=event_country_filter_qs.values('countries'),
).distinct()

from api.models import FieldReport


class Command(BaseCommand):
Copy link
Member

@thenav56 thenav56 Dec 31, 2024

Choose a reason for hiding this comment

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

Let's also monitor memory usages... or use iterator/bulk-managers during for loop

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