feat: import certifications into database #203
Conversation
|
|
||
| rail_line = | ||
| cond do | ||
| String.ends_with?(title, "BL") -> |
There was a problem hiding this comment.
@skyqrose heads up, this is a code smell, but was actually mentioned by LMS.
lib/orbit/import/import_helpers.ex
Outdated
| @@ -0,0 +1,63 @@ | |||
| defmodule Orbit.Import.ImportHelpers do | |||
There was a problem hiding this comment.
This file should probably have tests. It'd mostly be exercised by the tests of the individual importers, but those cases would be complex, so it'd be nice to have some simple tests for these functions, too.
There was a problem hiding this comment.
yeah, you're right. here are a couple for parse_csv—the other two functions are really just wrappers? https://github.com/mbta/orbit/pull/203/files#diff-b5202e24be21249b1155432d249572dd4a4b0a3b844f445fe4051215d31579deR1
mathcolo
force-pushed
the
pim-load-certifications
branch
from
9a2052a to
4ad04e8
Compare
mathcolo
force-pushed
the
pim-load-certifications
branch
from
4ad04e8 to
672afe1
Compare
|
@skyqrose Let me know what you think; I threw everything (except for the configuration) into |
| } | ||
| schema "certifications" do | ||
| field(:badge, :string) | ||
| field(:type, Ecto.Enum, values: [right_of_way: 0, rail: 1]) |
There was a problem hiding this comment.
Most places we use Ecto.Enum, we save the values as atoms (which get converted to strings in the database), though I see the Glides code this is copied from uses ints (it's the only place that uses ints).
Have you thought about whether you'd rather use ints or atoms/strings here?
There was a problem hiding this comment.
I haven't, but I am now...
Although I admit integers don't make a ton of sense for values that don't have an ordering, I suppose I'm not sure using a physically wide space for an enum makes more sense either :( Lose/lose, going to leave as in glides.
| :red | ||
|
|
||
| true -> | ||
| nil |
There was a problem hiding this comment.
Definitely not worth a splunk alert. I was thinking it might occasionally be useful if you see it locally or if doing a manual search for it while debugging an issue.
But I guess if we're manually debugging something, it's just as easy to grab the CSV from S3 and see the missing title there.
If we expect this to show up rarely or never, then might as well log it, that's what Log.warn is for, but it's definitely not necessary.
Asana Task: 📐 Load Orbit Cert expiries into DB
Do what Glides does to import certifications, except also include a
rail_linefield based on the certification title.Checklist
(x)Has tests( )Doesn't need tests( )Tests deferred (with justification)(x)Okayed the plan for the feature (e.g. the design files, or the Asana task)( )Reviewed the feature as implemented (e.g. on dev-green, or saw screenshots)( )No review needed