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

[DEV-11086] Add new PYA and PARK fields to Files B and C #4212

Open
wants to merge 8 commits into
base: qat
Choose a base branch
from

Conversation

collinwr
Copy link
Contributor

Description:
Broker is introducing 4 new fields to be to both their file b and c tables. We should update our corresponding tables and loaders to ensure these fields are read.

Technical details:
Field and table details can be found in ticket.

This should not be merged until broker adds these fields

Requirements for PR merge:

  1. Unit & integration tests updated - Made small change to the test_load_multiple_submissions test to ensure that the build will fail until broker adds these fields. Other than that, we don't have a comprehensive test that ensures that each individual field was successfully loaded from broker during the submission loader.
  2. N/A - API documentation updated
  3. Necessary PR reviewers:
    • Backend
  4. N/A - Matview impact assessment completed
  5. N/A - Frontend impact assessment completed
  6. Data validation completed
  7. Appropriate Operations ticket(s) created - Need to create deploy coordination for running migrations
  8. Jira Ticket DEV-11086:
    • Link to this Pull-Request
    • Performance evaluation of affected (API | Script | Download)
    • Before / After data comparison

Area for explaining above N/A when needed:

@collinwr collinwr added the do not merge [pr] This PR should not be merged label Oct 18, 2024
Comment on lines 20 to 21
prior_year_adjustment = models.TextField(blank=True, null=True)
pa_reporting_key = models.TextField(blank=True, null=True)

Choose a reason for hiding this comment

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

Do we expect a certain set of values for these fields (e.g. can we use choices)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choices is a good idea when we're accepting something from a source that hasn't been validated (maybe user submission). In this case, we're reading data from the broker, which has already performed it's own set of validations.

We could still potentially add options here, but if anything changes on the schema side (a new PARK is added, we'd need to make sure to update this before hand, or else the submission loader would fail).

I'd also suspect that in this case, there might be too many options to include here. We have quite a few program activities.

I can add help_text as Seth suggested though.

@@ -17,6 +17,8 @@ class AbstractFinancialAccountsByAwards(DataSourceTrackedModel):
parent_award_id = models.TextField(blank=True, null=True)
fain = models.TextField(blank=True, null=True)
uri = models.TextField(blank=True, null=True)
prior_year_adjustment = models.TextField(blank=True, null=True)
pa_reporting_key = models.TextField(blank=True, null=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there was a question about what pa_ stands for it might be good to make use of help_text here similar to this spot on Award model: https://github.com/fedspendingtransparency/usaspending-api/blob/master/usaspending_api/awards/models/award.py#L54

@@ -13,6 +13,8 @@ class AbstractFinancialAccountsByProgramActivityObjectClass(DataSourceTrackedMod
treasury_account = models.ForeignKey(
"accounts.TreasuryAppropriationAccount", models.CASCADE, related_name="program_balances", null=True
)
prior_year_adjustment = models.TextField(blank=True, null=True)
pa_reporting_key = models.TextField(blank=True, null=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar use of help_text here would be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge [pr] This PR should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants