Implement STI on User model #64959

nicklathe · 2025-04-01T23:41:05Z

This PR adds STI (Single Table Inheritance) to our User model. There are two new classes introduced, Teacher and Student that both inherit from the User base class. One benefit of implementing STI on the User model is that it allows us organize the model along the axis of user_type and move student and teacher specific logic into their respective classes.

This functionality should not break existing usage. For example, you can continue to create a new user by calling User.create(user_type: "Teacher"). You can also now instead call Teacher.create(), as STI provides additional class methods.

This PR adds the following changes:

Creates teacher.rb and student.rb and associated classes
Adds STI inheritance column override (default is type, this overrides that to be user_type
Adds find_sti_class method override
Adds logic to set the user class in user_builder.rb
Updates unit tests to handle the introduction of the new STI classes

Links

Testing story

You can test locally, and will be able to create new teacher and student accounts as before, as well as downgrade/upgrade account styles (Teacher -> Student, Student -> Teacher).

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

nicklathe · 2025-04-03T21:02:26Z

dashboard/test/controllers/home_controller_test.rb

-    assert_select '#user_age'
-    assert_select '#user_us_state', false
-    assert_select '#user_gender_student_input', false
+    assert_select '#student_age'


Rails prepends the user class for these attributes because the form builder knows the object is a User model. However since a user is now either a Teacher or Student class, these attributes are now being prepended with student_ rather than user_. This is probably obvious to those very familiar with Rails, but I was unaware of this prepending logic.

- Add STI definitions to user.rb - Add Student and Teacher classes - Fix unit tests in user_test.rb Signed-off-by: Nick Lathe <[email protected]>

- Partial registration user builder - Password resetter by email

dashboard/app/models/user.rb

dashboard/lib/services/partial_registration/user_builder.rb

dashboard/test/factories/factories.rb

- Move class mapping to find_sti_class method - Simplify factory

carl-codeorg · 2025-04-14T22:23:11Z

dashboard/app/models/user.rb

+  TYPE_TO_STI_CLASS_MAP = {
+    TYPE_TEACHER => ::Teacher,
+    TYPE_STUDENT => ::Student,
+    'staff' => ::Teacher # Powerschool sends through 'staff' instead of 'teacher'


Do we know if Powerschool SSO is still in use?

I don't know, I was just maintaining current functionality.

I think that feels like something we should do in a followup PR.

Yeah it can wait for sure. I'm running a few queries to see if we can remove this later.

carl-codeorg · 2025-04-15T20:45:49Z

Successfully tested locally using a variety of users. For Canvas, though, if I launch as a teacher for the first time, I get an email validation error on the finish signup screen. Doesn't happen on latest staging. Are you able to repro this?

nicklathe · 2025-04-15T21:19:32Z

Successfully tested locally using a variety of users. For Canvas, though, if I launch as a teacher for the first time, I get an email validation error on the finish signup screen. Doesn't happen on latest staging. Are you able to repro this?

Interesting. I'm working through some failing unit tests with my most recent PR feedback changes pushed. Will try to repro and validate this new bug. Thanks for testing and discovering this!

artem-vavilov · 2025-04-15T21:36:04Z

dashboard/lib/services/partial_registration/user_builder.rb

        user = ::User.new_with_session(user_params, session)
+
+        sti_class = ::User.find_sti_class(user_params[:user_type])
+        user = user.becomes!(sti_class) if sti_class


User#become! reinitializes the user instance, which breaks the tricky argument setup done by User.new_with_session:

Suggested change

user = ::User.new_with_session(user_params, session)

sti_class = ::User.find_sti_class(user_params[:user_type])

user = user.becomes!(sti_class) if sti_class

params = user_params

user_class = ::User.find_sti_class(params[:user_type]) || ::User

user = user_class.new_with_session(params, session)

Ah yes, this is the fix! Thanks @artem-vavilov!

artem-vavilov · 2025-04-15T22:04:06Z

dashboard/lib/services/partial_registration/user_builder.rb

@@ -32,6 +35,7 @@ def call
            user_params[:school_info_attributes].permit(:school_id, :school_name, :school_type, :zip, :country)
          end
        when ::User::TYPE_STUDENT
+          # binding.pry


Suggested change

# binding.pry

carl-codeorg

LTI teacher signup working for me now. Nice job on the quick fix.

artem-vavilov

@nicklathe Before merging this PR, please let me double-check the places where we manually convert the user instance into the required STI class, as it should work automatically

nicklathe · 2025-04-16T19:59:18Z

@nicklathe Before merging this PR, please let me double-check the places where we manually convert the user instance into the required STI class, as it should work automatically

@artem-vavilov I just approved your linked PR to merge into this branch.

Update User STI

artem-vavilov

Great work! 🙌🏻

This reverts commit 0735a8b.

nicklathe commented Apr 3, 2025

View reviewed changes

nicklathe added 6 commits April 7, 2025 07:50

Implement STI on User model

308052c

- Add STI definitions to user.rb - Add Student and Teacher classes - Fix unit tests in user_test.rb Signed-off-by: Nick Lathe <[email protected]>

Fix unit tests

fb2f43d

- Partial registration user builder - Password resetter by email

Ohly assign class if user_type is present

3451652

Fix hopefully final set of tests

41177a1

Update user_builder to create the right user class

1fad165

Recast class in unit tests that error

6fdd7c1

nicklathe force-pushed the nicklathe/user-sti-v2 branch from 8d26809 to 6fdd7c1 Compare April 7, 2025 14:58

Fix omniauth_controller_test unit tests

d57c838

nicklathe marked this pull request as ready for review April 10, 2025 02:58

nicklathe requested a review from a team April 10, 2025 23:16

artem-vavilov reviewed Apr 14, 2025

View reviewed changes

PR feedback changes

4de898f

- Move class mapping to find_sti_class method - Simplify factory

carl-codeorg reviewed Apr 14, 2025

View reviewed changes

artem-vavilov reviewed Apr 15, 2025

View reviewed changes

Create class then user off of new class

d3939b9

artem-vavilov reviewed Apr 15, 2025

View reviewed changes

carl-codeorg approved these changes Apr 15, 2025

View reviewed changes

artem-vavilov reviewed Apr 16, 2025

View reviewed changes

artem-vavilov added 3 commits April 16, 2025 09:50

Fix Services::PartialRegistration::UserBuilder

44749be

Update tests

da186d8

Fix FollowersController#student_register

c45f9ad

artem-vavilov mentioned this pull request Apr 16, 2025

Update User STI #65290

Merged

nicklathe and others added 2 commits April 16, 2025 09:12

Remove stray binding.pry

b493bcf

Fix User.find_or_create_teacher

f1111d5

Merge pull request #65290 from code-dot-org/user-sti-update

748ed11

Update User STI

artem-vavilov approved these changes Apr 16, 2025

View reviewed changes

nicklathe merged commit 0735a8b into staging Apr 17, 2025
6 checks passed

nicklathe deleted the nicklathe/user-sti-v2 branch April 17, 2025 17:34

nicklathe added a commit that referenced this pull request Apr 18, 2025

Revert "Implement STI on User model (#64959)"

b715a5d

This reverts commit 0735a8b.

nicklathe mentioned this pull request Apr 18, 2025

Revert "Implement STI on User model" #65362

Merged

nicklathe restored the nicklathe/user-sti-v2 branch April 23, 2025 21:03

Implement STI on User model #64959

Implement STI on User model #64959

Uh oh!

Conversation

nicklathe commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Uh oh!

nicklathe Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carl-codeorg Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

nicklathe Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

nicklathe Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

carl-codeorg Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

carl-codeorg commented Apr 15, 2025

Uh oh!

nicklathe commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artem-vavilov Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicklathe Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

artem-vavilov Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

carl-codeorg left a comment

Choose a reason for hiding this comment

Uh oh!

artem-vavilov left a comment

Choose a reason for hiding this comment

Uh oh!

nicklathe commented Apr 16, 2025

Uh oh!

artem-vavilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicklathe commented Apr 1, 2025 •

edited

Loading

nicklathe Apr 3, 2025 •

edited

Loading

nicklathe commented Apr 15, 2025 •

edited

Loading

artem-vavilov Apr 15, 2025 •

edited

Loading