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

Kma 163213435 write import fuel function #1793

Merged
merged 50 commits into from Mar 5, 2019

Conversation

4 participants
@kimallen
Copy link
Contributor

kimallen commented Feb 27, 2019

Description

This PR gets the fuel values for the current month through the EIA Open Data api
Finds the earliest posted fuel data for the current month (if it exists), calculates the fuel percentage, and updates the eia_diesel_fuel_prices table with the appropriate values. It is set to check that we have records for the past 12 months.

This function will be used to run a regularly scheduled task to keep the fuel prices up to date. There is a CLI command to run this function for now (see setup).

Reviewer Notes

We don't currently account for if the fuel price is less than the base price ($2.50) and this is set up to error out if that is so. This sets fuel percentage to 0, as indicated in documentation, in the case that the base price is >= the fuel price

The way this is currently written, we check for missing rows within the last 12 months. This can be adjusted for anything under 12 months.

WIP - Tests are in progress. . . Tests written. Any cases I'm missing?

I am not erroring if the first Monday of the month has passed and there is no data for that month- should I handle that situation? Now handling this

I'm getting a month's data and choosing the earliest pub_date in that data, assuming that is the first Monday of the month (the record we use). I did write a function to find the first Monday of the month- would it be better to just fetch data for that date?

Setup

Add the EIA_KEY to your .envrc.local

make db_dev_e2e_populate

Remove the record in eia_diesel_fuel_prices with the most recent pub_date. Also remove a pub_date from within the last 12 months, and one more than 12 months out.

go run cmd/save_fuel_price_data/main.go

Check the database- the two records you removed from the last 12 months should be there now. The one you removed from more than 12 months ago will not be replaced.

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

kimallen added some commits Feb 6, 2019

Replace references to time.Now() with clock; Make fetch of last few m…
…onths more generic and pass in number of months
Account for error returned as successful json response; add error han…
…dling for possible un-desired responses and other error handling
Add fetch funciton as part of struct and start mock function for test…
…s; add error handling of calculation; move structs to top of file
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #1793 into master will increase coverage by 0.02%.
The diff coverage is 73.15%.

@@            Coverage Diff             @@
##           master    #1793      +/-   ##
==========================================
+ Coverage   49.09%   49.11%   +0.02%     
==========================================
  Files         426      427       +1     
  Lines       18194    18264      +70     
  Branches     1630     1630              
==========================================
+ Hits         8932     8971      +39     
- Misses       8482     8504      +22     
- Partials      780      789       +9
@reggieriser
Copy link
Contributor

reggieriser left a comment

Nice job working through all this data processing/manipulation! I left some comments -- I think there are a few possible bugs that need fixing, but noted some other style-related items that don't necessarily have to be changed.

I didn't get to review the tests yet, but wanted to go ahead and get this feedback to you.

Show resolved Hide resolved cmd/save_fuel_price_data/main.go
func checkEIAKey(v *viper.Viper) error {
eiaKey := v.GetString("eia-key")
if len(eiaKey) != 32 {
return fmt.Errorf("eia key is not 32 characters key is %d chars", len(eiaKey))

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

Nit: Error message reads awkwardly -- needs punctuation or rewording.

This comment has been minimized.

@kimallen

kimallen Mar 2, 2019

Author Contributor

adjusted

Show resolved Hide resolved cmd/webserver/main_test.go
// FetchMostRecentFuelPrices queries and fetches all fuel_eia_diesel_prices for past specified number of months
func FetchMostRecentFuelPrices(dbConnection *pop.Connection, clock clock.Clock, numMonths int) ([]FuelEIADieselPrice, error) {
today := clock.Now()
query := dbConnection.Where("pub_date >= $1 AND pub_date <= $2", today.AddDate(0, -numMonths, 0), today)

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

Does timezone factor in here? I believe clock.Now() is going to give you local time, not UTC.

Just an FYI ... I think you can also use something like pub_date BETWEEN $1 AND $2 which may read a little better (but that's really just a style choice).

This comment has been minimized.

@kimallen

kimallen Mar 2, 2019

Author Contributor

added UTC to this and the test clocks. I do wonder if we should factor what time zone the fuel data is updated in the api? Probably doesnt really matter- if the function is run daily, we know that if it's not retrieved the day it's posted, it would be retrieved the next day.

This comment has been minimized.

@reggieriser

reggieriser Mar 4, 2019

Contributor

Agreed -- it probably won't ultimately matter in this case.

today := clock.Now()
query := dbConnection.Where("pub_date >= $1 AND pub_date <= $2", today.AddDate(0, -numMonths, 0), today)
// TODO: what if today's date is before the first Monday/publication date, but in the new month?
var fuelPrices []FuelEIADieselPrice

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

There's already an FuelEIADieselPrices type for this if you want to use it.

}
dateString = monthFuelData[weekIndex][0].(string)
price = monthFuelData[weekIndex][1].(float64)
} else if len(monthFuelData) == 1 {

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

Just noting that I think you could just make the first if condition test for >= 1 and this case would be handled OK there instead of having to being special-cased? No need to change.

This comment has been minimized.

@kimallen

kimallen Mar 4, 2019

Author Contributor

refactored

isWorkMondayOrNonHolidayAfter := false

for isWorkMondayOrNonHolidayAfter == false {
if dayToCheck.Weekday() == time.Monday && cal.IsWorkday(dayToCheck) {

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

Suppose that Monday is Labor Day. Is this going to get the Tuesday after it? Seems like it may get the following Monday instead because we don't stop until it's a Monday and a workday.

This comment has been minimized.

@kimallen

kimallen Mar 2, 2019

Author Contributor

Funny- I accounted for that, thought it was wrong, and then removed it. Too much thinking. Adding it back.

return false
}

func getFirstMondayOrNonHolidayAfter(date time.Time, clock clock.Clock) time.Time {

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

Doesn't look like clock is used here.

This comment has been minimized.

@kimallen

kimallen Mar 4, 2019

Author Contributor

removed

func getFirstMondayOrNonHolidayAfter(date time.Time, clock clock.Clock) time.Time {
// loop through days of month until you hit a non-holiday Monday or first workday after
cal := dates.NewUSCalendar()
dayToCheck := now.New(date).BeginningOfMonth()

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

Actually, it looks like you already pass in the first day of the month as the date parameter to the function. If that's what you expect the function to always get, no need for BeginningOfMonth after all.

pricePerGallon := fuelValues.price
pubDateString := fuelValues.dateString
year, err := strconv.Atoi(pubDateString[:4])
if err != nil {

This comment has been minimized.

@reggieriser

reggieriser Mar 1, 2019

Contributor

It might be a little cleaner to use time.Parse to convert the string to a Time and handle any errors once there. Then you could just pull out the parts you need using Year(), Month(), etc. calls on the (known good) Time object.

This comment has been minimized.

@kimallen

kimallen Mar 4, 2019

Author Contributor

done

This comment has been minimized.

@kimallen

kimallen Mar 4, 2019

Author Contributor

Done

@kimallen kimallen added the A-Team label Mar 3, 2019

kimallen added some commits Mar 3, 2019

Change type of OtherData to accomodate unknown data; add test for non…
…-Monday publish date; refactor datestring conversion using time.Parse
expectedNumFuelPrices = 12
suite.NoError(err)
suite.Equal(expectedNumFuelPrices, len(fuelPrices))
// TODO: another test to make sure earliest and latest month match?

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 4, 2019

Contributor

nit: Add the test or remove the TODO :)

This comment has been minimized.

@kimallen

kimallen Mar 4, 2019

Author Contributor

removed- I don't even understand what I meant there

}
monthString := fmt.Sprintf("%02s", strconv.Itoa(month))
startDateString = fmt.Sprintf("%v%v%v", year, monthString, startDay)
endDateString = fmt.Sprintf("%v%v%v", year, monthString, endDay)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 4, 2019

Contributor

I'm not a huge fan of using %v. It works but it's better to do type checking by using %d or %s where you know what the type is in advance.

This comment has been minimized.

@kimallen

kimallen Mar 5, 2019

Author Contributor

replaced with more specific types

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

At this point I have mostly nits for you. I'd like to see you use a logger passed in as opposed to the default logger. And the rest is up to you.

@reggieriser
Copy link
Contributor

reggieriser left a comment

Adding a few more comments. Want to review a little more to make sure I've hit all questions and glance over tests before approving.

month := time.Month(monthInt)
day, err := strconv.Atoi(pubDateString[6:])
pubDateFormatString := pubDateString[:4] + "-" + pubDateString[4:6] + "-" + pubDateString[6:] + "T00:00:00Z"
pubDate, err := time.Parse(time.RFC3339, pubDateFormatString)

This comment has been minimized.

@reggieriser

reggieriser Mar 4, 2019

Contributor

I think you can avoid building up pubDateFormatString and just use pubDate, err := time.Parse("20060102", pubDateString). That 20060102 part is showing how the "reference time" (as defined by the time.Parse docs) would look like in that format. Go playground: https://play.golang.org/p/i1IlVXFZS2J

This comment has been minimized.

@kimallen

kimallen Mar 4, 2019

Author Contributor

Cool that that works. I scoured the docs to see if that format would be recognized, but didn't find anything on it.

Show resolved Hide resolved pkg/services/fuelprice/diesel_fuel_price_storer_test.go
// FetchMostRecentFuelPrices queries and fetches all fuel_eia_diesel_prices for past specified number of months
func FetchMostRecentFuelPrices(dbConnection *pop.Connection, clock clock.Clock, numMonths int) ([]FuelEIADieselPrice, error) {
today := clock.Now()
query := dbConnection.Where("pub_date >= $1 AND pub_date <= $2", today.AddDate(0, -numMonths, 0), today)

This comment has been minimized.

@reggieriser

reggieriser Mar 4, 2019

Contributor

Agreed -- it probably won't ultimately matter in this case.

kimallen added some commits Mar 5, 2019

Move tests into subtests; add comments to explain formula used to get…
… fuel baseline rate; fix layout date for parsing datestring

@kimallen kimallen requested review from chrisgilmerproj and reggieriser Mar 5, 2019

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

Nice job!

@reggieriser

This comment has been minimized.

Copy link
Contributor

reggieriser commented Mar 5, 2019

LGTM! 🚢

@kimallen kimallen merged commit b838bb3 into master Mar 5, 2019

17 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
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_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp 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
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
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.