Update schools table #1318

david-mears-2 · 2020-02-13T11:12:41Z

Jira ticket URL:

https://dfedigital.atlassian.net/browse/TEVA-28

Changes in this PR:

Perform migration to add columns to table schema
Updated the update_schools_data.rb to set values of the new columns

We wanted to provide this data for the business analysts. The columns will be used for upcoming key info features.

We changed the update_school_data.rb to set the new information that we want based on the migration in the earlier commit. This information will be used by the business analysts for school-based metrics. We also want to surface some of this new information to job-seekers to help them make a decision.

Renny4Real · 2020-02-13T11:33:12Z

db/migrate/20200212150214_add_key_info_to_schools.rb

@@ -0,0 +1,17 @@
+class AddKeyInfoToSchools < ActiveRecord::Migration[5.2]
+  def change
+    add_column :schools, :status, :string


Wow. These are a lot of columns to add at once. 😄

It's based on the columns needed for bigquery reporting and those needed for the key information tickets, wanted to avoid performing multiple migrations if possible 😄

So, I'm going to beat my usual drum on this one. Can you please put them in alphabetical order by the name of the column?

I have no issue with the number of them...

And be aware (mainly for @david-mears-dfe's benefit) - schema.rb will change as well.

Renny4Real · 2020-02-13T11:44:11Z

lib/update_school_data.rb

@@ -29,6 +29,7 @@ def convert_to_school(row)
    school
  end

+  # rubocop:disable Metrics/AbcSize


Not an issue but I think you can remove the need to disable this if you move all the new stuff into a separate method and call the method within set properties.

def set_properties(school, row) school.name = row['EstablishmentName'] school.address = row['Street'] school.locality = row['Locality'].presence school.address3 = row['Address3'].presence school.town = row['Town'] school.county = row['County (name)'].presence school.postcode = row['Postcode'] school.local_authority = row['LA (name)'] school.minimum_age = row['StatutoryLowAge'] school.maximum_age = row['StatutoryHighAge'] school.easting = row['Easting'] school.northing = row['Northing'] school.url = valid_website(row['SchoolWebsite']) school.phase = row['PhaseOfEducation (code)'].to_i set_school_key_info(school, row) end def set_school_key_info(school, row) school.status = row['EstablishmentStatus (name)'] school.trust_name = row['Trusts (name)'] school.number_of_pupils = row['NumberOfPupils'] school.head_title = row['HeadTitle (name)'] school.head_first_name = row['HeadFirstName'] school.head_last_name = row['HeadLastName'] school.religious_character = row['ReligiousCharacter (name)'] school.rsc_region = row['RSCRegion (name)'] school.telephone = row['TelephoneNum'] school.open_date = row['OpenDate'] school.close_date = row['CloseDate'] school.last_ofsted_inspection_date = row['OfstedLastInsp'] school.oftsed_rating = row['OfstedRating (name)'] end

Not all those columns are needed for the key info snippets, we decided to put it in one method based on export_vacancy_records_to_big_query.rb and just disable to metric in order to keep it all in one place

These long lists of import attributes are difficult to read and maintain and fragile.

Can you please replace the hardcoded list of keys/attributes with an abstract method that maps keys of known format to symbols suitable for importing and have the migration follow the same schema. Please make the attribute names follow the names used by the API (so EstablishmentName becomes school.establishment_name - you can alias a school.name public method to it to expose that for internal use).

Renny4Real · 2020-02-13T11:46:00Z

spec/lib/update_school_data_spec.rb

@@ -119,6 +132,19 @@
        expect(school.northing).to eql('181201')
        expect(school.geolocation.x).to be_within(0.0000000000001).of(51.51396894535262)
        expect(school.geolocation.y).to be_within(0.0000000000001).of(-0.07751626505544208)
+        expect(school.status).to eql('Open')


Could these expectancies be refactored into a method?

what kind of method did you have in mind?

Not sure. Sorry for replying late @dushan-madetech
These lines of expectations are in two different tests, I think.
So I was wondering if there was a way to extract all the expectancies into a method and then call the method in the tests so that we could reduce the number of lines. 🤔

Renny4Real

Left some comments. Looks good!

tatyree

My two main issues are that long lists of individually-assigned attributes are fragile and a code smell. Please refactor so the attributes:
a) use the same name API attribute they are pulled from (at least as we store them, we can alias the stored version using public methods if we need a different name).
and
b) are alphabetised wherever we have to manually maintain that list.

tatyree · 2020-02-13T12:49:15Z

db/migrate/20200212150214_add_key_info_to_schools.rb

@@ -0,0 +1,17 @@
+class AddKeyInfoToSchools < ActiveRecord::Migration[5.2]
+  def change
+    add_column :schools, :status, :string


So, I'm going to beat my usual drum on this one. Can you please put them in alphabetical order by the name of the column?

I have no issue with the number of them...

And be aware (mainly for @david-mears-dfe's benefit) - schema.rb will change as well.

tatyree · 2020-02-13T12:55:07Z

spec/factories/schools.rb

@@ -14,6 +14,19 @@
    phase { :secondary }
    easting { '1' }
    northing { '1' }
+    status { 'Open' }


And again...sing with me now al-pha-bet-i-cal...

tatyree · 2020-02-13T13:06:38Z

lib/update_school_data.rb

@@ -29,6 +29,7 @@ def convert_to_school(row)
    school
  end

+  # rubocop:disable Metrics/AbcSize


These long lists of import attributes are difficult to read and maintain and fragile.

Can you please replace the hardcoded list of keys/attributes with an abstract method that maps keys of known format to symbols suitable for importing and have the migration follow the same schema. Please make the attribute names follow the names used by the API (so EstablishmentName becomes school.establishment_name - you can alias a school.name public method to it to expose that for internal use).

david-mears-2 added 2 commits February 13, 2020 11:02

Created migration for key info

90009fb

We wanted to provide this data for the business analysts. The columns will be used for upcoming key info features.

cesidio approved these changes Feb 13, 2020

View reviewed changes

Renny4Real reviewed Feb 13, 2020

View reviewed changes

dushan-madetech merged commit 8d7a71a into develop Feb 13, 2020

dushan-madetech deleted the add-data-to-schools-table-from-gias branch February 13, 2020 12:55

tatyree suggested changes Feb 13, 2020

View reviewed changes

david-mears-2 restored the add-data-to-schools-table-from-gias branch February 13, 2020 13:59

dushan-madetech mentioned this pull request Feb 13, 2020

Revert "Update schools table" #1320

Merged

tatyree deleted the add-data-to-schools-table-from-gias branch March 3, 2020 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update schools table #1318

Update schools table #1318

david-mears-2 commented Feb 13, 2020

Renny4Real Feb 13, 2020

dushan-madetech Feb 13, 2020

tatyree Feb 13, 2020

Renny4Real Feb 13, 2020

dushan-madetech Feb 13, 2020

tatyree Feb 13, 2020

Renny4Real Feb 13, 2020

dushan-madetech Feb 13, 2020

Renny4Real Feb 13, 2020

Renny4Real left a comment

tatyree left a comment

tatyree Feb 13, 2020

tatyree Feb 13, 2020

tatyree Feb 13, 2020

Update schools table #1318

Update schools table #1318

Conversation

david-mears-2 commented Feb 13, 2020

Jira ticket URL:

Changes in this PR:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renny4Real left a comment

Choose a reason for hiding this comment

tatyree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment