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 importers for management, counseling services, task order fees, a… #3292

Merged
merged 15 commits into from Jan 15, 2020

Conversation

@LeDeep
Copy link
Contributor

LeDeep commented Jan 8, 2020

…nd shipment types - tabs 4a and 5a on pricing template

Description

This PR imports management and counseling services (tab 4a of pricing template), domestic and international task order fees and shipment types (tab 5a) and price escalations (tab 5b).

Reviewer Notes

In order to pull in all the data, I needed to add a couple more map helpers contractYearToIDMap and shipmentTypeToIDMap.

I also added a migration to change the type for factor_hundredths to be a float, instead of an int.

Setup

To do the import:

  • rm bin/ghc-pricing-parser
  • make bin/ghc-pricing-parser
  • make db_dev_reset db_dev_migrate
  • ghc-pricing-parser --filename /path/to/fake/pricing/template.xlsx --contract-code TEST

After running this, you will see records in the following tables:
re_shipment_type_prices
re_contract_years
re_domestic_accessorial_prices
re_intl_accessorial_prices
re_task_order_fees

Please spot check db data against the pricing template.

To run the tests:

  • make server_test

References

…nd shipment types - tabs 4a and 5a on pricing template
@LeDeep LeDeep added the A-Team label Jan 9, 2020
Copy link
Contributor

chrisgilmerproj left a comment

🚀 - as far as the migration it looks good to me.

Copy link
Contributor

kimallen left a comment

Overall, looks good! Thanks for working through so many tables. Just the comments that you've mostly already addressed.

The only blocker is to make changes to use re_services instead of re_shipment_types (the clean-up migration for the re_shipment_types table could go in this or a different PR)

I also have the remaining question as to how best to handle the hard-coded start/end dates (I'm curious to get another opinion).

Where("service_id = $2", serviceNotValid.ID).
Where("services_schedule = $3", 3).
First(&domesticAccessorialPriceServiceNotValid)
suite.Error(err)

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 14, 2020

Contributor

Optional: I think you could do a table-driven test here and cut out a lot of the code repetition.

Copy link
Contributor

reggieriser left a comment

That's a lot of importing! Good job sticking with this one. I saw a few things that I think we need to sort out before approval. Some comments I marked as "optional" or "nit", so it's your call to do anything on those or not.

…e both origin and destination services where required
Copy link
Contributor

reggieriser left a comment

Nice changes -- we're almost there! Blocking on some cases where we should probably throw an error if we can't find a service description string (I think we'd just silently ignore it with the current code). Other than that, just a couple of nits.

} else {
return fmt.Errorf("service provided [%s] is not a valid service", stageDomesticAccessorialPrice.ServiceProvided)
}
for _, service := range services {

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 15, 2020

Contributor

If we get through this loop and we have no match in this slice for the given stageDomesticAccessorialPrice.ServiceProvided, I'd think we'd want to error rather than silently skip the line.

This comment has been minimized.

Copy link
@LeDeep

LeDeep Jan 15, 2020

Author Contributor

added error check

} else {
return fmt.Errorf("market [%s] is not a valid market", stageIntlAccessorialPrice.Market)
}
for _, service := range services {

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 15, 2020

Contributor

Same thing here about throwing an error instead of silently failing if we don't have a stageIntlAccessorialPrice.ServiceProvided match after the loop is complete.

This comment has been minimized.

Copy link
@LeDeep

LeDeep Jan 15, 2020

Author Contributor

added error check

@@ -17,45 +17,54 @@ func (gre *GHCRateEngineImporter) importREShipmentTypePrices(dbTx *pop.Connectio
return fmt.Errorf("could not read staged domestic international additional prices: %w", err)
}

shipmentCodePositionInSlice := 0
//shipmentCodePositionInSlice := 0

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 15, 2020

Contributor

Leftover comment?

This comment has been minimized.

Copy link
@LeDeep

LeDeep Jan 15, 2020

Author Contributor

yep, removed

if verrs.HasAny() {
return fmt.Errorf("error saving ReShipmentTypePrices: %+v with validation errors: %w", shipmentTypePrice, verrs)

for shipmentType, serviceCode := range serviceToCodeMap {

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 15, 2020

Contributor

Another case where we probably should throw an error when no match.

This comment has been minimized.

Copy link
@LeDeep

LeDeep Jan 15, 2020

Author Contributor

added error check

intlAccessorial.ServiceID = serviceIDSHUT
} else {
return fmt.Errorf("service provided [%s] is not a valid service", stageIntlAccessorialPrice.ServiceProvided)
services := []struct {

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 15, 2020

Contributor

Another case of a slice that's constant and can be moved out of the loop.

This comment has been minimized.

Copy link
@LeDeep

LeDeep Jan 15, 2020

Author Contributor

moved out of loop

ContractID: gre.contractID,
ServicesSchedule: servicesSchedule,
PerUnitCents: unit.Cents(perUnitCentsService),
services := []struct {

This comment has been minimized.

Copy link
@reggieriser

reggieriser Jan 15, 2020

Contributor

I'm not sure if this gets optimized away by the Go compiler, but this slice is constant so it can be moved out of the loop.

This comment has been minimized.

Copy link
@LeDeep

LeDeep Jan 15, 2020

Author Contributor

moved out of loop

Copy link
Contributor

kimallen left a comment

Thanks for all your work and modifications on this!

Side comment: I noticed that the original PR only had one commit. It's helpful as a reviewer to see a story through multiple commits, and I recommend committing more often as you write the code as good practice.

@reggieriser

This comment has been minimized.

Copy link
Contributor

reggieriser commented Jan 15, 2020

Looks good! Thanks for making the changes. I re-ran the ghc-pricing-parser again and verified that the counts looked right in the tables affected by this PR.

@LeDeep LeDeep merged commit 789a6be into master Jan 15, 2020
16 checks passed
16 checks passed
auto-approve dependabot PRs
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_storybook_app Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: check_generated_code Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
@LeDeep LeDeep deleted the sm-MB-474-import-4a-5a branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.