Composite key in AP school codes #27866

bencodeorg · 2019-04-03T22:56:51Z

This PR updates model relationships to reflect that AP school codes are now unique at the [school_year, school_code] level.

edit: in the process of this PR, the file ap_school_codes.csv was (manually) moved from the bucket cdo-census into a folder called ap_school_codes inside of cdo-census, and was renamed 2016-2017.csv.

sureshc · 2019-04-04T17:18:06Z

dashboard/app/models/census/ap_school_code.rb

@@ -55,7 +56,7 @@ def self.seed_from_s3

  def self.seed
    if CDO.stub_school_data
-      seed_from_csv("test/fixtures/census/ap_school_codes.csv")
+      seed_from_csv("test/fixtures/census/ap_school_codes.csv", 2016)


Could we borrow the convention from the state course offerings seeding logic where the school year is in the filename and picked up dynamically?

Sure, I can make that change. Any thoughts on how to test this change more broadly?

Ended up doing what you suggested here. I tested the impact of moving the existing ap_school_code.csv file to a place like "ap_school_codes/#{school_year}-#{school_year + 1}.csv" by:

copying down existing ap_school_codes table from production

doing complete seeding of ap_school_codes using files in new location

checking whether any rows with school_year = 2016 (ie, existing data) had a modified updated_at (they did not).

bencodeorg · 2019-05-31T22:43:21Z

dashboard/test/factories/census_factories.rb

      ap_cs_offering {build_list(:ap_cs_offering, 1, school_year: school_year)}
    end
  end

  factory :ap_cs_offering, class: 'Census::ApCsOffering' do
-    ap_school_code {build :ap_school_code}
+    ap_school_code {build :ap_school_code, school_year: school_year}


Adding school_year here resolved ActiveRecord issues we were having when testing.

bencodeorg · 2019-05-31T22:45:32Z

dashboard/app/models/census/census_summary.rb

-    ap_offerings_this_year = ap_offerings.select {|o| o.school_year == school_year}
-    ap_offerings_this_year.each do |offering|
-      audit[:ap_cs_offerings].push(offering.id)
+    ap_offerings_this_year = []


Once we resolved the ActiveRecord error in the ap_school_code factory, we resolved a real test failure that resulted from the prior assumption that each school had only one ap_school_code. Now a school can have a different ap_school_code per year.

bencodeorg · 2019-05-31T23:04:14Z

Two months later... @uponthesun helped get this across the finish line! @sureshc as a neutral party could you take a look at this again and see if it looks good to you?

sureshc · 2019-06-04T20:54:44Z

So awesome that @uponthesun helped you solve the problem with the Factory!

Would it be good to have tests that exercise the two Associations to School and ApCsOffering? They depend on a gem to be able to establish relationships with ApSchoolCode through the composite key. Might be handy to make sure the Association through the composite key continues to work even when the gem is upgraded in the future or other foundational components (like ActiveRecord) change in the future. Should we have a test that creates two ApSchoolCodes that point to the same school and two different ApCsOfferings that use those two school codes?

bencodeorg · 2019-07-08T23:50:17Z

@sureshc forgot about this PR, but there's a bit of a fire drill around the access report this week that I'll need to resolve this for...I think what you are suggesting is sort of happening in the test that @uponthesun and I were working on. This test:

https://github.com/code-dot-org/code-dot-org/blob/staging/dashboard/test/models/census/census_summary_test.rb#L175

Makes sure we can create an ApSchoolCode and ApCsOffering for a given School -- it doesn't quite do what you're suggesting (make sure that a single School can have multiple ApSchoolCodes), but I think does at least make sure that the base functionality (ie, joining between models via composite key) works?

bencodeorg · 2019-07-10T21:05:07Z

@sureshc I was being lazy, took your advice and added a test per your comment (in this commit).

I also committed code changes required to ingest the new mappings and AP data in this commit. I might want to break that out into a different PR though, per @Erin007's presentation yesterday.

sureshc · 2019-07-10T21:39:00Z

dashboard/test/models/census/census_summary_test.rb

@@ -142,6 +142,22 @@ class Census::CensusSummaryTest < ActiveSupport::TestCase
    assert Census::CensusSummary.submission_teaches_cs?(submission, is_high_school: nil, is_k8_school: nil)
  end

+  test "School with different AP school codes in different years functions correctly" do


This is what I was thinking of! Test ability to create two ap school codes for the same school for different years with different codes. I think these tests belong in ap_school_code_test.rb though.

I was thinking of putting it in census_school_test.rb, but that doesn't exist -- what assertion would you suggest using then (instead of checking the summarization)? FWIW, the validate_summary approach I used here actually caught a problem.

bencodeorg · 2019-07-12T16:22:45Z

dashboard/test/models/census/ap_school_code_test.rb

+    refute school_code.valid?
+  end
+
+  test "Composite key relationship between AP school code and AP CS offering" do


@sureshc added another test that mirrors some of the scenarios you whiteboarded yesterday. I have three assertions here -- should I split into different tests?

sureshc · 2019-07-12T17:11:04Z

dashboard/app/models/census/ap_school_code.rb

+        school_id = School.normalize_school_id(row.to_hash['school_id'])
+        school = School.find_by(id: school_id)
+        if school.nil?
+          puts "AP School Code seed: school not found - skipping row for school_code:#{normalized_school_code} school_id:#{school_id}"


Suggested change

puts "AP School Code seed: school not found - skipping row for school_code:#{normalized_school_code} school_id:#{school_id}"

CDO.log.warn "AP School Code seed: school not found - skipping row for school_code:#{normalized_school_code} school_id:#{school_id}"

sureshc · 2019-07-12T17:16:54Z

dashboard/app/models/census/ap_school_code.rb

+    "ap_school_codes/#{school_year}-#{school_year + 1}.csv"
+  end
+
+  def self.seed_from_csv(filename, school_year)
    ActiveRecord::Base.transaction do


I know this functionality isn’t changing from what already existed, but I don’t think there’s any benefit to wrapping the CSV import in a transaction. Do we really want to ROLLBACK all of the INSERTed rows if a single row encounters an error? There’s a likely small database performance penalty, as well, for carrying out all of the INSERTs in a single transaction.

sureshc · 2019-07-12T17:18:00Z

dashboard/app/models/census/ap_cs_offering.rb

@@ -45,7 +46,7 @@ def self.seed_from_csv(course, school_year, filename)
          rescue ActiveRecord::RecordNotFound
            # We don't have mapping for every school code so skip over any that
            # can't be found in the database.
-            puts "AP CS Offering seeding: skipping unknown school code #{normalized_school_code}"
+            puts "AP CS Offering seeding: skipping unknown school code #{normalized_school_code}, school name #{raw_school_name}"


Suggested change

puts "AP CS Offering seeding: skipping unknown school code #{normalized_school_code}, school name #{raw_school_name}"

CDO.log.warn "AP CS Offering seeding: skipping unknown school code #{normalized_school_code}, school name #{raw_school_name}"

sureshc

Looks great! Definitely change the puts to CDO.log.warn before merging.

bencodeorg · 2019-07-12T19:05:12Z

Switched out puts statements in both ApSchoolCode and ApCsOffering models -- for the record, as the great Shaggy once said, it wasn't me.

bencodeorg added 2 commits April 3, 2019 15:05

Update AP school code model relationships

2990c9b

Modify school code import steps to allow distinct files by school year

020dfb0

bencodeorg requested review from sureshc and hacodeorg April 3, 2019 23:15

sureshc reviewed Apr 4, 2019

View reviewed changes

Add tests for updated AP school code model

8e0f44b

bencodeorg changed the title ~~[WIP, DO NOT MERGE] Composite key in AP school codes~~ Composite key in AP school codes Apr 8, 2019

bencodeorg added 5 commits April 9, 2019 17:00

Allow multiple AP school codes per school

e490877

Merge branch 'staging' into composite-key-in-ap-school-codes

fcd8898

Merge branch 'staging' into composite-key-in-ap-school-codes

07f0994

Include school year when using AP school code factory

cd7fd65

Add support for new composite key in census summary logic

1c298fe

bencodeorg requested review from uponthesun and removed request for hacodeorg May 31, 2019 21:32

bencodeorg commented May 31, 2019

View reviewed changes

bencodeorg added 3 commits July 10, 2019 13:48

Add test for school with different AP school codes in different years

71e20db

Seed AP school codes and AP CS offerings for 2017-18

1024fa3

Merge branch 'staging' into composite-key-in-ap-school-codes

4b5c94e

sureshc reviewed Jul 10, 2019

View reviewed changes

sureshc self-requested a review July 11, 2019 21:02

bencodeorg added 2 commits July 11, 2019 18:05

Add school name to error message when seeding AP CS offerings

9e9b1f5

Add tests for composite key between ApSchoolCode and ApCsOffering

82d46d5

bencodeorg commented Jul 12, 2019

View reviewed changes

sureshc reviewed Jul 12, 2019

View reviewed changes

sureshc approved these changes Jul 12, 2019

View reviewed changes

Switch out puts statements

0a983e7

bencodeorg merged commit 9cbf900 into staging Jul 12, 2019

bencodeorg mentioned this pull request Jul 19, 2019

Add primary key to AP school code model description #29804

Merged

bencodeorg deleted the composite-key-in-ap-school-codes branch July 29, 2019 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composite key in AP school codes #27866

Composite key in AP school codes #27866

bencodeorg commented Apr 3, 2019 •

edited

sureshc Apr 4, 2019

bencodeorg Apr 5, 2019

bencodeorg Jul 12, 2019

bencodeorg May 31, 2019

bencodeorg May 31, 2019

bencodeorg commented May 31, 2019

sureshc commented Jun 4, 2019 •

edited

bencodeorg commented Jul 8, 2019

bencodeorg commented Jul 10, 2019

sureshc Jul 10, 2019

bencodeorg Jul 10, 2019

bencodeorg Jul 12, 2019

sureshc Jul 12, 2019

sureshc Jul 12, 2019

sureshc Jul 12, 2019

sureshc left a comment

bencodeorg commented Jul 12, 2019

	puts "AP School Code seed: school not found - skipping row for school_code:#{normalized_school_code} school_id:#{school_id}"
	CDO.log.warn "AP School Code seed: school not found - skipping row for school_code:#{normalized_school_code} school_id:#{school_id}"

	puts "AP CS Offering seeding: skipping unknown school code #{normalized_school_code}, school name #{raw_school_name}"
	CDO.log.warn "AP CS Offering seeding: skipping unknown school code #{normalized_school_code}, school name #{raw_school_name}"

Composite key in AP school codes #27866

Composite key in AP school codes #27866

Conversation

bencodeorg commented Apr 3, 2019 • edited

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

bencodeorg commented May 31, 2019

sureshc commented Jun 4, 2019 • edited

bencodeorg commented Jul 8, 2019

bencodeorg commented Jul 10, 2019

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

sureshc left a comment

Choose a reason for hiding this comment

bencodeorg commented Jul 12, 2019

bencodeorg commented Apr 3, 2019 •

edited

sureshc commented Jun 4, 2019 •

edited