Skip to content
This repository has been archived by the owner on Dec 22, 2022. It is now read-only.

Automating human readable strings for Discovery by reading Symphony configs #1897

Merged
merged 8 commits into from
Mar 12, 2020

Conversation

pgwillia
Copy link
Member

@pgwillia pgwillia commented Feb 4, 2020

Working towards automating changes to the human readable strings for the 'Where is this?' table.
Screenshot from 2020-01-10 11-10-23

With data/data4discovery in place
Invoke: rails g symphony_nightly

The things that this PR proposes to change

  • create backup tables for library and location
  • prepare for incoming library short codes
  • denormalize library locations
  • add symphony_nightly generator

I'd suggest that the reviewer look at each commit individually because I tried to group them into atomic units of change. I also tried to add information about why and what in the commit messages. I guess they could be different pull requests. I'm open to feedback on how to present a change like this.

#1843

Some answers

  • confirmed that data/data4discovery.txt and data/Data4DiscoveryManual.txt are the correct name/place for the symphony config dump
  • are more handling for error conditions necessary? Not now.
  • should the generator take a parameter for the file location? Not now. Could at some point.

@pgwillia pgwillia requested a review from a user February 4, 2020 21:28
@pgwillia pgwillia force-pushed the 1843_symphony_nightly branch 3 times, most recently from 2449c60 to bc07285 Compare February 4, 2020 21:58
db/migrate/20200128205653_change_library_short_code.rb Outdated Show resolved Hide resolved
@@ -81,6 +81,8 @@ group :test do

# Easy installation and use of chromedriver to run system tests with Chrome
gem 'chromedriver-helper'

gem 'generator_spec', '~> 0.9.4'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generator-spec allowed me to test my symphony_nightly generator with RSpec

app/models/backup_location.rb Outdated Show resolved Hide resolved
app/models/backup_location.rb Outdated Show resolved Hide resolved
@pgwillia pgwillia force-pushed the 1843_symphony_nightly branch from 67f6ef2 to f4347ce Compare February 4, 2020 23:03
@pgwillia pgwillia force-pushed the 1843_symphony_nightly branch from ef4f4a3 to 74b33f8 Compare February 6, 2020 17:13
murny
murny previously approved these changes Feb 6, 2020
Copy link
Contributor

@murny murny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good 👍

Wonder if it's worth adding a couple of tests to the backup_libraries/backup_locations models? Would have caught that foreign_key missing column, etc. Would be super simple tests since no real validations except for the belongs_to.

Otherwise code looks good

@pgwillia pgwillia requested a review from murny February 7, 2020 20:26
@pgwillia pgwillia force-pushed the 1843_symphony_nightly branch 6 times, most recently from c044bdb to f7b655d Compare February 14, 2020 19:07
ghost
ghost previously approved these changes Feb 14, 2020
@ghost
Copy link

ghost commented Feb 14, 2020 via email

@pgwillia
Copy link
Member Author

Make short_code and name columns null:false

@pgwillia
Copy link
Member Author

pgwillia commented Mar 3, 2020

After discussion with Elaine a couple of changes were made. The format of the ezproxy config will change and be in a seperate file. Waiting for email receipt of the file to continue work.

pgwillia added 3 commits March 5, 2020 11:48
Working towards automating the nightly populating of these tables.  I'd
like to have a backup for a couple reasons:
1. if something goes sideways during the migration we can just restore the old tables
2. there is some data in the table which cannot be automated (urls, relationships between location and library)

For reason 2 I'm also adding ActiveRecord classes to help navigate the backup data.
These need to exist before the symphony_nightly generator is run even if they're
empty to begin with.

- magic of Rails means that the table names do NOT need to be specified
- belongs_to is smart about foreign key names
- added tests to have confidence that I was invoking the magic correctly
Working towards automating changes to the human readable strings for
the 'Where is this?' table. Locations belong to a Library.  I'm not sure
of the origin of the current short codes but it looks like it's been
a challenge to deal with ualbertalib/NEOSDiscovery@4da3e71#diff-e678c8aa2cbba5c5a1251bc942a2182a

There are some values that we have in our tables currently (url, neos_url and proxy)
which don't have an analog in the Symphony configuration. If the Symphony
configuration is driving the nightly update then we'll need a way to map
between these two names for the same thing.

I've put together this table to illustrate what I'm thinking makes sense.
I've joined the libraries based on a common proxy url between the export
provided and the existing configuration.
|LIBG|proxy|existing|
|--|--|--|
|ALL_LIBS
|UALB_CAT
|VANGUARD_|VANGUARD|vanguard|
|AGL_|AGINTERNET http://login.ezproxy.library.ualberta.ca/login?url=|universityofalberta|
|AHS_|AHSGLENRSE http://ahs.idm.oclc.org/login?url=|abhealth|
|AITF_||innovates|
|CONCORDIA_|CONCORDIA http://aec.talonline.ca/login?url=|concordia|
|COVENANT_|COVENANTGN http://ahs.idm.oclc.org/login?url=,|covenant|
|UALBERTA2|
|BUR_|CUC http://ezproxy.achcu.talonline.ca/login?url=|burmanuniversity|
|GPRC_|GPRC_GP https://ezproxy.agpc.talonline.ca/login?url=|gp|
|MACEWAN_|GR_MACEWAN http://ezproxy.macewan.ca/login?url=|macewanuniversity|
|NEOS_FREE_||neos|
|KEYANO_|KEYANO http://ezproxy.afmk.talonline.ca/login?url=|keyano|
|KINGS_|KINGS http://aekc.talonline.ca/login?url=|kings|
|LAKELAND_|LAKELND_VR http://ezproxy.avc.talonline.ca/login?url=|lakeland|
|NEWMAN_|NEWMAN http://ezproxy.newman.edu:81/login|newman|
|OLDS_|OLDS http://ezproxy.aoac.talonline.ca/login?url=|olds|
|RDC|RED_DEER_C http://ardc.talonline.ca/login?url=|reddeer|
|TAYLOR_|
|UINTERNET_|
|NORQUEST_|NORQ_MAIN http://auth01.norquest.ca/login?url=|norquest|
|NLC_|NLC_SL http://ezproxy.asav.talonline.ca/login?url=|nlc|
|ALL_LIBS2|
|UARCRF_|
|UALBERTA|
||ARC_MW|
|||gov https://ezproxy.aee.talonline.ca/login?url=|
|||free|

I'm keeping the old ones because they might be necessary in NEOSDiscovery.
Working towards automating changes to the human readable strings for
the 'Where is this?' table.

There is one value in our table currently (url) which doesn't have an
analog in the Symphony configuration. If the Symphony configuration is
driving the nightly update then we'll need a way to map between these
two names for the same thing.

My observation is that the normalization that's being done isn't adding
any value (needs conversion on both sides), so we might as well just
store the value that Symphony is using.
pgwillia added 3 commits March 5, 2020 11:49
Working towards automating changes to the human readable strings for
the 'Where is this?' table.  Creates a new database migration from the
current symphony config data dump found at `data/data4discovery`. It can
be invoked `rails g symphony_nightly`

This will:
1. rename the existing table so it can be used as backup
2. insert all the values found in the file
3. carry forward some data and relationships between locations/libraries
add symphony_nightly generator
4. allow for `db:rollback` for one night of recovery
likely missed in rebase and caused failing tests.  Also revert searches column size just in case.
The Data4DiscoveryManual.txt file will contain the relationships between library locations and their NEOS library parent so we don't need to access the BackupLocations and BackupLibraries.  The library short codes are also not changing so the migration to do so is no longer necessary.

Also some minor tweaks to the erb template so that formatting is nicer.
@pgwillia pgwillia force-pushed the 1843_symphony_nightly branch from f7b655d to be5d571 Compare March 5, 2020 22:25
@pgwillia pgwillia requested review from a user, ConnorSheremeta, mbarnett and lagoan March 6, 2020 17:13
@pgwillia
Copy link
Member Author

pgwillia commented Mar 6, 2020

I made the changes in response to the change of file. Ready for re-review.

@pgwillia pgwillia requested a review from mbarnett March 6, 2020 18:42
Copy link

@mbarnett mbarnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

@@ -1,8 +1,13 @@
class Location < ActiveRecord::Base
belongs_to :library

# These are the locations that indicate an electronic resource available to ualberta
UNIVERSITY_OF_ALBERTA_INTERNET = 'UAINTERNET'.freeze
NEOS_FREE_INTERNET_RESOURCES = 'NEOS_FREE'.freeze

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good comment and naming! 👏

@pgwillia pgwillia merged commit 8dc22c1 into master Mar 12, 2020
@pgwillia pgwillia deleted the 1843_symphony_nightly branch March 12, 2020 16:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants