search

pulibrary / locations

Services around holding, delivery, and physical locations
MIT License
3 stars 0 forks source link

87 add remote storage #88

Closed christinach closed 3 years ago

christinach commented 3 years ago

closes #87

This pr will add a new remote storage attribute, update the views and the gems.

christinach commented 3 years ago

However when I test this Pr in my local bibdata and I run rails generate locations:install, I see that it creates 2 older migration files that probably had never run before. I noticed that in bibdata starting 2017 there were migrations to update the location tables directly in the bibdata database. So even though I spent some time to update the gems in this PR I'm thinking that it would be better to add the migration to add the remote_storage attribute directly in bibdata and not in the locations gem.

christinach commented 3 years ago

ok, I looked again the migration files and they seem ok to me based on the dates. for reference these are the 2 migration files that existed in locations but not in bibdata: https://github.com/pulibrary/locations/blob/87-add-remote-storage/db/migrate/20171213175117_remove_locations_library_from_locations_floors.rb https://github.com/pulibrary/locations/blob/87-add-remote-storage/db/migrate/20171213175341_drop_locations_floors.rb

As far as I know we don't use the locations floors and it would be ok to remove them.

christinach commented 3 years ago

What is the reasoning behind disallowing nil and defaulting to / storing the empty string? I would have expected to use nil when there's no value.

I thought about that too. I added an empty string because in the api call we either have a remote storage location value or it is an empty string which means it is not in a remote storage. If it is nil I would think that it expects a value which would be either an empty string or a remote storage. I wanted the default value to be not in a remote_storage.

christinach commented 3 years ago

I'm ok either or. I dont have a strong opinion. @carolyncole based on your experience checking in the requests the attribute values that are coming from the holding locations do you think it would be better to keep the default value as an empty string or as nil?

christinach commented 3 years ago

based on a discussion in slack with @carolyncole and @kevinreiss requests are checking for blank? for the holding location attributes and it covers both nil and empty string. Even though we set default values in the rest of the attributes and we don't let them nil I will change the remote_storage to be nil based on @hackartisan question and @kevinreiss confirmation.