diff --git a/app/controllers/relationships_controller.rb b/app/controllers/relationships_controller.rb index b59e1fbb..773def13 100644 --- a/app/controllers/relationships_controller.rb +++ b/app/controllers/relationships_controller.rb @@ -21,7 +21,7 @@ def create user.subscribe_to_section(section) render(json: { msg: "Subscribed" }) else - user.follow(section) + user.follow_section(section) render(json: { msg: "Followed" }) end end diff --git a/app/models/relationship.rb b/app/models/relationship.rb index 01f8b4c5..1279e60c 100644 --- a/app/models/relationship.rb +++ b/app/models/relationship.rb @@ -8,8 +8,49 @@ class Relationship < ApplicationRecord belongs_to :section has_one :review, dependent: :restrict_with_exception + enum stored_status: { + planned: "planned", + subscribed: "subscribed", + enrolled: "enrolled", + } + + class Status < T::Enum + enums do + # A relationship can be in one of the following states: + Planned = new("planned") # A user has put this section into their class plan for a quarter + Subscribed = new("subscribed") # The section is in a class plan and the user wants enrollment notifications for the section + Enrolled = new("enrolled") # The section has been enrolled in. No need to send further notifications. + + Completed = new("completed") # Where enrolled sections go after the quarter ends. The user has taken the course! + Reviewed = new("reviewed") # The user has taken the section and reviewed it. + + # How does Status differ from stored_status above? We track the first three states (planned, subscribed, enrolled) in the database on this relationship model. The other two states (completed, reviewed) are derived based off other information (term calendar and if a review was written, respectively). + end + end + + sig { returns(Status) } + def status + if reviewed? + Status::Reviewed + elsif completed? + Status::Completed + else + Status.deserialize(stored_status) + end + end + sig { returns(T::Boolean) } - def review? + def reviewed? review.present? end + + sig { returns(T::Boolean) } + def completed? + section&.term&.enrollment_period == Term::EnrollmentPeriod::Post + end + + sig { returns(T::Boolean) } + def reviewed_or_completed? + reviewed? || completed? + end end diff --git a/app/models/user.rb b/app/models/user.rb index b40eaee3..6c2713fe 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -113,23 +113,25 @@ def self.from_auth_payload(payload, referral_code:) # Marks a section as being followed by the given user. sig { params(section: Section).void } - def follow(section) + def follow_section(section) sections << section unless following?(section) end # Marks a section as being followed with notifications by the given user. sig { params(section: Section).void } def subscribe_to_section(section) - follow(section) - relationship = relationships.find_by(section:) - relationship&.update(notify: true) if use_notification_token + follow_section(section) + relationship = relationships.find_by!(section:) + raise ArgumentError, "Section cannot be subscribed to, it's past enrollment" if relationship.completed? + + relationship.subscribed! if use_notification_token end # Whether a user is subscribed to a given section. sig { params(section: Section).returns(T::Boolean) } def subscribed_to_section?(section) relationship = relationships.find_by(section:) - relationship&.notify? || false + !!relationship&.subscribed? end # Whether a user is following a given section. @@ -138,11 +140,17 @@ def following?(section) sections.include?(section) end + sig { params(section: Section).returns(Relationship::Status) } + def section_status(section) + relationship = relationships.find_by!(section:) + relationship.status + end + # Stops following a section the user was following. sig { params(section: Section).void } def unfollow(section) relationship = relationships.find_by(section:) - raise ActiveRecord::RecordNotDestroyed, "Section is reviewed by user, cannot unfollow." if relationship&.review? + raise ActiveRecord::RecordNotDestroyed, "Section is reviewed by user, cannot unfollow." if relationship&.reviewed? sections.delete(section) end diff --git a/db/data/20230827195318_move_notify_boolean_to_relationship_status.rb b/db/data/20230827195318_move_notify_boolean_to_relationship_status.rb new file mode 100644 index 00000000..13ffd4ca --- /dev/null +++ b/db/data/20230827195318_move_notify_boolean_to_relationship_status.rb @@ -0,0 +1,20 @@ +# typed: true +# frozen_string_literal: true + +class MoveNotifyBooleanToRelationshipStatus < ActiveRecord::Migration[7.0] + def up + Relationship.all.find_each do |relationship| + # We have no way of telling whether a user enrolled in a section -- that's a new state! + relationship.stored_status = if relationship.notify + :subscribed + else + :planned + end + relationship.save! + end + end + + def down + Relationship.update_all(stored_status: nil) + end +end diff --git a/db/data_schema.rb b/db/data_schema.rb new file mode 100644 index 00000000..06bce58d --- /dev/null +++ b/db/data_schema.rb @@ -0,0 +1 @@ +DataMigrate::Data.define(version: 20230827195318) diff --git a/db/migrate/20230827154404_track_relationship_status.rb b/db/migrate/20230827154404_track_relationship_status.rb new file mode 100644 index 00000000..eeaafb07 --- /dev/null +++ b/db/migrate/20230827154404_track_relationship_status.rb @@ -0,0 +1,25 @@ +# typed: true +# frozen_string_literal: true + +class TrackRelationshipStatus < ActiveRecord::Migration[7.0] + def change + reversible do |dir| + dir.up do + execute(<<-SQL.squish) + CREATE TYPE relationship_status AS ENUM ('planned', 'subscribed', 'enrolled'); + SQL + end + + dir.down do + execute(<<-SQL.squish) + DROP TYPE relationship_status; + SQL + end + end + + change_table(:relationships, bulk: true) do |t| + # Will make this non-null in a future migration + t.column(:stored_status, :relationship_status, default: "planned") + end + end +end diff --git a/db/structure.sql b/db/structure.sql index a2cc8464..25dc7427 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -74,6 +74,17 @@ CREATE TYPE public.grade_type AS ENUM ( ); +-- +-- Name: relationship_status; Type: TYPE; Schema: public; Owner: - +-- + +CREATE TYPE public.relationship_status AS ENUM ( + 'planned', + 'subscribed', + 'enrolled' +); + + -- -- Name: review_status; Type: TYPE; Schema: public; Owner: - -- @@ -521,6 +532,15 @@ CREATE TABLE public.courses_terms ( ); +-- +-- Name: data_migrations; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.data_migrations ( + version character varying NOT NULL +); + + -- -- Name: enrollment_appointments; Type: TABLE; Schema: public; Owner: - -- @@ -970,7 +990,8 @@ CREATE TABLE public.relationships ( section_id bigint NOT NULL, created_at timestamp(6) without time zone NOT NULL, updated_at timestamp(6) without time zone NOT NULL, - notify boolean DEFAULT false NOT NULL + notify boolean DEFAULT false NOT NULL, + stored_status public.relationship_status DEFAULT 'planned'::public.relationship_status ); @@ -1633,6 +1654,14 @@ ALTER TABLE ONLY public.courses ADD CONSTRAINT courses_pkey PRIMARY KEY (id); +-- +-- Name: data_migrations data_migrations_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.data_migrations + ADD CONSTRAINT data_migrations_pkey PRIMARY KEY (version); + + -- -- Name: enrollment_appointments enrollment_appointments_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -2517,6 +2546,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20230430161016'), ('20230511033013'), ('20230718192047'), -('20230820174337'); +('20230820174337'), +('20230827154404'); diff --git a/sorbet/rbi/dsl/relationship.rbi b/sorbet/rbi/dsl/relationship.rbi index cd121626..70f6de1f 100644 --- a/sorbet/rbi/dsl/relationship.rbi +++ b/sorbet/rbi/dsl/relationship.rbi @@ -7,6 +7,7 @@ class Relationship include GeneratedAssociationMethods include GeneratedAttributeMethods + include EnumMethodsModule extend CommonRelationMethods extend GeneratedRelationMethods @@ -15,6 +16,11 @@ class Relationship sig { returns(NilClass) } def to_ary; end + class << self + sig { returns(T::Hash[T.any(String, Symbol), String]) } + def stored_statuses; end + end + module CommonRelationMethods sig { params(block: T.nilable(T.proc.params(record: ::Relationship).returns(T.untyped))).returns(T::Boolean) } def any?(&block); end @@ -263,6 +269,26 @@ class Relationship def third_to_last!; end end + module EnumMethodsModule + sig { void } + def enrolled!; end + + sig { returns(T::Boolean) } + def enrolled?; end + + sig { void } + def planned!; end + + sig { returns(T::Boolean) } + def planned?; end + + sig { void } + def subscribed!; end + + sig { returns(T::Boolean) } + def subscribed?; end + end + module GeneratedAssociationMethods sig { params(args: T.untyped, blk: T.untyped).returns(::Review) } def build_review(*args, &blk); end @@ -338,6 +364,9 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def eager_load(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } + def enrolled(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def except(*args, &blk); end @@ -423,6 +452,15 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def none(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } + def not_enrolled(*args, &blk); end + + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } + def not_planned(*args, &blk); end + + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } + def not_subscribed(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def offset(*args, &blk); end @@ -438,6 +476,9 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def order(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } + def planned(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def preload(*args, &blk); end @@ -468,6 +509,9 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def structurally_compatible?(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } + def subscribed(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) } def uniq!(*args, &blk); end @@ -647,6 +691,9 @@ class Relationship sig { void } def restore_section_id!; end + sig { void } + def restore_stored_status!; end + sig { void } def restore_updated_at!; end @@ -677,6 +724,12 @@ class Relationship sig { returns(T::Boolean) } def saved_change_to_section_id?; end + sig { returns(T.nilable([T.nilable(::String), T.nilable(::String)])) } + def saved_change_to_stored_status; end + + sig { returns(T::Boolean) } + def saved_change_to_stored_status?; end + sig { returns(T.nilable([T.nilable(::ActiveSupport::TimeWithZone), T.nilable(::ActiveSupport::TimeWithZone)])) } def saved_change_to_updated_at; end @@ -734,6 +787,51 @@ class Relationship sig { void } def section_id_will_change!; end + sig { returns(T.nilable(::String)) } + def stored_status; end + + sig { params(value: T.nilable(T.any(::String, ::Symbol))).returns(T.nilable(T.any(::String, ::Symbol))) } + def stored_status=(value); end + + sig { returns(T::Boolean) } + def stored_status?; end + + sig { returns(T.nilable(::String)) } + def stored_status_before_last_save; end + + sig { returns(T.untyped) } + def stored_status_before_type_cast; end + + sig { returns(T::Boolean) } + def stored_status_came_from_user?; end + + sig { returns(T.nilable([T.nilable(::String), T.nilable(::String)])) } + def stored_status_change; end + + sig { returns(T.nilable([T.nilable(::String), T.nilable(::String)])) } + def stored_status_change_to_be_saved; end + + sig { returns(T::Boolean) } + def stored_status_changed?; end + + sig { returns(T.nilable(::String)) } + def stored_status_in_database; end + + sig { returns(T.nilable([T.nilable(::String), T.nilable(::String)])) } + def stored_status_previous_change; end + + sig { returns(T::Boolean) } + def stored_status_previously_changed?; end + + sig { returns(T.nilable(::String)) } + def stored_status_previously_was; end + + sig { returns(T.nilable(::String)) } + def stored_status_was; end + + sig { void } + def stored_status_will_change!; end + sig { returns(T.nilable(::ActiveSupport::TimeWithZone)) } def updated_at; end @@ -836,6 +934,9 @@ class Relationship sig { returns(T::Boolean) } def will_save_change_to_section_id?; end + sig { returns(T::Boolean) } + def will_save_change_to_stored_status?; end + sig { returns(T::Boolean) } def will_save_change_to_updated_at?; end @@ -862,6 +963,9 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def eager_load(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } + def enrolled(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def except(*args, &blk); end @@ -913,6 +1017,15 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def none(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } + def not_enrolled(*args, &blk); end + + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } + def not_planned(*args, &blk); end + + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } + def not_subscribed(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def offset(*args, &blk); end @@ -931,6 +1044,9 @@ class Relationship sig { params(num: T.nilable(Integer)).returns(ActiveRecord::Relation) } def page(num = nil); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } + def planned(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def preload(*args, &blk); end @@ -961,6 +1077,9 @@ class Relationship sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def structurally_compatible?(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } + def subscribed(*args, &blk); end + sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) } def uniq!(*args, &blk); end diff --git a/test/factories.rb b/test/factories.rb index 8f43693e..a77268cb 100644 --- a/test/factories.rb +++ b/test/factories.rb @@ -80,15 +80,6 @@ end end - factory :relationship do - user - section - - trait :with_review do - review - end - end - factory :review do relationship user diff --git a/test/factories/relationship.rb b/test/factories/relationship.rb new file mode 100644 index 00000000..0a13fe7b --- /dev/null +++ b/test/factories/relationship.rb @@ -0,0 +1,18 @@ +# typed: false +# frozen_string_literal: true + +FactoryBot.define do + factory :relationship do + user + section + stored_status { "planned" } + + trait :with_review do + review + end + + traits_for_enum :stored_status, planned: "planned", + subscribed: "subscribed", + enrolled: "enrolled" + end +end diff --git a/test/models/relationship_test.rb b/test/models/relationship_test.rb new file mode 100644 index 00000000..b46f95b0 --- /dev/null +++ b/test/models/relationship_test.rb @@ -0,0 +1,22 @@ +# typed: strict +# frozen_string_literal: true + +require "test_helper" + +class RelationshipTest < ActiveSupport::TestCase + describe "#status" do + # it "does something" + end + + describe "reviewed?" do + it "returns true if the relationship has a review" do + relationship = create(:relationship, :with_review) + assert_predicate(relationship, :reviewed?) + end + + it "returns false if the relationship does not have a review" do + relationship = create(:relationship) + assert_not_predicate(relationship, :reviewed?) + end + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 42202e25..c379c4c2 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -4,38 +4,145 @@ require "test_helper" class UserTest < ActiveSupport::TestCase - it "always has a name" do - valid_user = build(:user, name: "Nathan Smith") - assert_predicate valid_user, :valid? + describe "validations" do + it "always has a name" do + valid_user = build(:user, name: "Nathan Smith") + assert_predicate valid_user, :valid? - invalid_user = build(:user, name: "") - assert_not invalid_user.valid? - end + invalid_user = build(:user, name: "") + assert_not invalid_user.valid? + end - it "always has an email" do - valid_user = build(:user, email: "natedub@g.ucla.edu") - assert_predicate valid_user, :valid? + it "always has an email" do + valid_user = build(:user, email: "natedub@g.ucla.edu") + assert_predicate valid_user, :valid? - invalid_user = build(:user, name: "") - assert_not invalid_user.valid? - end + invalid_user = build(:user, name: "") + assert_not invalid_user.valid? + end + + it "has a name that is not too long" do + user = build(:user, name: "Nathan" * 10) + assert_not user.valid? + end - it "has a name that is not too long" do - user = build(:user, name: "Nathan" * 10) - assert_not user.valid? + it "has an email that is not too long" do + user = build(:user, email: "nathan.smith@ucla.edu" * 1500) + assert_not user.valid? + end + + it "can only have a ucla email" do + valid_user = build(:user, email: "nathansmith@g.ucla.edu") + assert_predicate valid_user, :valid? + + invalid_user = build(:user, email: "nathansmith@gmail.com") + assert_not invalid_user.valid? + end end - it "has an email that is not too long" do - user = build(:user, email: "nathan.smith@ucla.edu" * 1500) - assert_not user.valid? + describe "relationships" do + before do + @term = T.let(create_current_term, Term) + travel_to_active_enrollment + end + + describe "#follow_section" do + it "adds a section to a user's sections" do + user = T.let(create(:user), User) + section = T.let(create(:section, term: @term), Section) + + assert_not_includes(user.sections, section) + + user.follow_section(section) + assert_includes(user.sections, section) + end + + it "does nothing if a section is already followed" do + user = T.let(create(:user), User) + section = T.let(create(:section, term: @term), Section) + create(:relationship, section:, user:) + + assert_includes(user.sections, section) + + user.follow_section(section) + assert_includes(user.sections, section) + end + end + + describe "#subscribe_to_section" do + it "subscribes a user to a section they follow and takes a token" do + user = T.let(create(:user, notification_token_count: 1), User) + section = T.let(create(:section, term: @term), Section) + create(:relationship, section:, user:) + + assert_includes(user.sections, section) + + user.subscribe_to_section(section) + assert_includes(user.sections, section) + assert_equal(user.section_status(section), Relationship::Status::Subscribed) + assert_equal(0, user.notification_token_count) + end + + it "follows a section (if not followed), sets the status to subscribed, and takes a token" do + user = T.let(create(:user, notification_token_count: 1), User) + section = T.let(create(:section, term: @term), Section) + + assert_not_includes(user.sections, section) + + user.subscribe_to_section(section) + assert_includes(user.sections, section) + assert_equal(user.section_status(section), Relationship::Status::Subscribed) + assert_equal(0, user.notification_token_count) + end + + it "can move an enrolled section to subscribed and takes a token" do + user = T.let(create(:user, notification_token_count: 1), User) + section = T.let(create(:section, term: @term), Section) + create(:relationship, :enrolled, section:, user:) + + assert_includes(user.sections, section) + + user.subscribe_to_section(section) + assert_includes(user.sections, section) + assert_equal(user.section_status(section), Relationship::Status::Subscribed) + assert_equal(0, user.notification_token_count) + end + + it "throws an error if the section is completed" do + travel_to_post_enrollment + user = T.let(create(:user, notification_token_count: 1), User) + section = T.let(create(:section, term: @term), Section) + create(:relationship, :enrolled, section:, user:) + + assert_includes(user.sections, section) + + assert_raises(ArgumentError) { user.subscribe_to_section(section) } + end + end + + describe "#enroll_in_section" do + it "marks a subscription a user is following to enrolled" do + # TODO: implement + end + + it "marks a subscription a user has subscribed to as enrolled" do + # TODO: implement + end + + it "follows and enrolls in a section a user does not follow" do + # TODO: implement + end + end end - it "can only have a ucla email" do - valid_user = build(:user, email: "nathansmith@g.ucla.edu") - assert_predicate valid_user, :valid? + describe "#subscribed_to_section?" do + it "returns true when a user is subscribed to the section" do + # TODO: implement + end - invalid_user = build(:user, email: "nathansmith@gmail.com") - assert_not invalid_user.valid? + it "returns false if the user is not subscribed to the section" do + # TODO: implement + end end describe "#unfollow" do diff --git a/test/test_helpers/term_helper.rb b/test/test_helpers/term_helper.rb index 812263a7..5e1b5ba6 100644 --- a/test/test_helpers/term_helper.rb +++ b/test/test_helpers/term_helper.rb @@ -11,7 +11,17 @@ def create_current_term term = create(:term, term: "21S", start_date: Date.new(2021, 3, 29), end_date: Date.new(2021, 6, 11)) - travel_to(Time.zone.local(2021, 5, 14)) + travel_to_post_enrollment term end + + sig { void } + def travel_to_active_enrollment + travel_to(Time.zone.local(2021, 3, 30)) + end + + sig { void } + def travel_to_post_enrollment + travel_to(Time.zone.local(2021, 5, 14)) + end end