Refactor fairly static text into ActiveRecord Models #1742
Conversation
Generated by 🚫 Danger |
pgwillia
changed the title
|
Something is up with my db:migration. I'll take a look tomorrow, I guess still a WIP. |
pgwillia
changed the title
pgwillia
changed the title
The I think this is ready for review. |
There was a problem hiding this comment.
This looks good to me. But I wouldn't mind @mbarnett and @redlibrarian's review and opinions.
| @@ -17,7 +17,21 @@ def holdings(document, method) | |||
| end | |||
|
|
|||
| def symphony_status(item) | |||
| SYMPHONY_STATUSES[item[:status].downcase.underscore.to_sym] | |||
| Status.find_by!(short_code: item[:status].downcase.underscore).name | |||
There was a problem hiding this comment.
what does find_by! do, difference with find_by?
There was a problem hiding this comment.
find_by! will throw an exception if there are no results, whereas find_by will return nil
There was a problem hiding this comment.
Not 100% sure that seeds are the right way to load this data (not 100% sure they're the wrong way, either, I'm just not sure what "db:seed will seed the current data (This will be important information when this gets handed to @nmacgreg )" means).
If this is data that's only being loaded when spinning up fresh databases, seeds are a good approach. If this data is going to be added to an existing production DB, doing it in a migration instead is probably going to be cleaner.
| SYMPHONY_STATUSES[item[:status].downcase.underscore.to_sym] | ||
| Status.find_by!(short_code: item[:status].downcase.underscore).name | ||
| rescue ActiveRecord::RecordNotFound | ||
| 'Unknown' |
There was a problem hiding this comment.
Related to Weiwei's comment, I just want to sanity check this approach since I don't know the exact details of the problem being solved here:
If 'Unknown' is expected to come up regularly, then we're using exceptions for control flow here and it might be better to do something like
Status.find_by(short_code: item[:status].downcase.underscore).name || 'Unknown'instead.
If this is just to provide some kind of sane default in a scenario we don't expect to happen only in case of some bug where something has a broken status (ie, if this is expected to be an exceptional circumstance), then this totally makes sense.
Either way works, it just depends on which intention we want to signal to future readers.
There was a problem hiding this comment.
We had decided "if an id is looked up that doesn't exist catch in rollbar and use 'Unknown' temporarily" #1273 (comment). Looks like I missed the Rollbar part. I'll leave it as an exception.
There was a problem hiding this comment.
That makes sense, just wanted to be sure.
It didn't occur to me that loading data in a migration was even a thing. It does make sense to do it that way because our intention is that the data is a capture from a point in time and will become out of date. If it's a seed then the expectation would be that it should be kept up. |
Status, ItemType, CirculationRule, Library, Location and Institution by `rails g model`
app/helpers/holdings_helper.rb:45 containing the declaration for the UAL_SHIELD_LIBRARIES constant that uses the Locations activerecord object is loaded by the migration potentially before it is created.
- moved db content creation to migration - added rollbar message to unknown exception rescues - prefer #detect to returns within #each
These tables will populate information in this table
And the UAL shield in the results
Out of scope:
#1273