feat(users): validation flashes for batch city and referrer (#318)

* feat(users): validate that referrer exists

* feat(users): validate city can't change

* feat(users): validate batch can't change

* fix(users): rspec

* fix(users): revert refactos

* fix(users) disable Metrics/ClassLength for user model

* fix(users): rename cant to cannot

* fix(users): move referrer param inside params

* feat(users): add explanation for params[:batch_id] = nil if form_params[:batch_number]

* refactor(users): validations params

* Spacing

* Handle referral_code in a proper update_referrer method (we keep referrer_code unpermitted) and drop the flash message for the batch, it's not worth the effort (after a looooong thought and tests). Forbidding City update is important (and easy) because scoring depends on it. Displaying a proper error message when a user tries a wrong referral code is worth it.

* Add back the batch validation

* Rename update to assign referrer

* Wording

* Rename, again

* Trigger batch/city validations on update

* Rollback to the previous behaviour: referrer cannot be nil after it was set

* Test pass, but behaviour is not as expected

* fix(users): add #to_i to referrer_id to trigger change even if user has no referrer yet

* Explicitly use -1 if referrer is not found

---------

Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com>

.rubocop.yml

@@ -13,6 +13,10 @@ Lint/MissingSuper:
 Lint/SymbolConversion:
   EnforcedStyle: consistent
 
+Metrics/ClassLength:
+  Exclude:
+    - "**/app/models/user.rb"
+
 Naming/VariableNumber:
   EnforcedStyle: snake_case
   Exclude:

app/controllers/users_controller.rb

@@ -46,7 +46,12 @@ def edit
   end
 
   def update
-    if current_user.update(updated_params)
+    current_user.batch_id = -1 if params.dig(:user, :batch_number) # Set impossible value to trigger validation
+
+    referrer_code = params.dig(:user, :referrer_code)
+    current_user.referrer_id = User.find_by_referral_code(referrer_code)&.id || -1 if referrer_code
+
+    if current_user.update(user_params)
       unlock_achievements
       redirect_back fallback_location: "/", notice: "Your user information was updated"
     else
@@ -85,30 +90,19 @@ def authenticate_admin
   end
 
   def restrict_after_lock
-    return unless Time.now.utc > Aoc.lock_time && (form_params[:entered_hardcore] == "1") != current_user.entered_hardcore
+    return unless Time.now.utc > Aoc.lock_time && (user_params[:entered_hardcore] == "1") != current_user.entered_hardcore
 
     redirect_back(
       fallback_location: "/",
       alert: "You cannot join or leave the Ladder of Insanity since #{Aoc.lock_time.to_fs(:long_ordinal)} (see FAQ)"
     )
   end
 
-  def updated_params
-    {
-      accepted_coc: form_params[:accepted_coc],
-      aoc_id: form_params[:aoc_id],
-      entered_hardcore: form_params[:entered_hardcore],
-      username: form_params[:username],
-      city_id: form_params[:city_id],
-      referrer: User.find_by_referral_code(form_params[:referrer_code])
-    }.compact
-  end
-
   def unlock_achievements
     Achievements::UnlockJob.perform_later(:city_join, current_user.id)
   end
 
-  def form_params
-    params.require(:user).permit(:accepted_coc, :aoc_id, :entered_hardcore, :username, :city_id, :referrer_code)
+  def user_params
+    params.require(:user).permit(:accepted_coc, :aoc_id, :city_id, :entered_hardcore, :username)
   end
 end

app/models/user.rb

@@ -26,10 +26,12 @@ class User < ApplicationRecord
   validates :aoc_id, numericality: { in: 1...(2**31), message: "should be between 1 and 2^31" }, allow_nil: true
   validates :aoc_id, uniqueness: { allow_nil: true }
   validates :username, presence: true
-
   validates :private_leaderboard, presence: true
 
-  validate :not_referring_self
+  validate :batch_cannot_be_changed,           on: :update, if: :batch_id_changed?
+  validate :city_cannot_be_changed_if_present, on: :update, if: :city_id_changed?
+  validate :referrer_must_exist,               on: :update, if: :referrer_id_changed?
+  validate :referrer_cannot_be_self,           on: :update
 
   scope :admins, -> { where(uid: ADMINS.values) }
   scope :confirmed, -> { where(accepted_coc: true, synced: true).where.not(aoc_id: nil) }
@@ -39,27 +41,25 @@ class User < ApplicationRecord
   before_validation :assign_private_leaderboard, on: :create
 
   def self.from_kitt(auth)
-    batches = auth.info&.schoolings
-    oldest_batch = batches.min_by { |batch| batch.camp.starts_at }
+    oldest_batch = auth.info&.schoolings&.min_by { |batch| batch.camp.starts_at }
 
     user = find_or_initialize_by(provider: auth.provider, uid: auth.uid) do |u|
       u.username = auth.info.github_nickname
+
       u.batch = Batch.find_or_initialize_by(number: oldest_batch&.camp&.slug.to_i)
       u.city = City.find_or_initialize_by(name: oldest_batch&.city&.name)
     end
 
     user.github_username = auth.info.github_nickname
 
     user.save
-
     user
   end
 
   def self.find_by_referral_code(code)
-    uid = code&.gsub(/R0*/, "")&.to_i
-    return if uid.nil?
+    return unless code&.match?(/R\d{5}/)
 
-    User.find_by(uid:)
+    User.find_by(uid: code.gsub(/R0*/, "").to_i)
   end
 
   def admin?
@@ -113,8 +113,20 @@ def referrer_code
 
   private
 
-  def not_referring_self
-    errors.add(:referrer, "must not be you") if referrer == self
+  def batch_cannot_be_changed
+    errors.add(:batch, "can't be changed")
+  end
+
+  def city_cannot_be_changed_if_present
+    errors.add(:city, "can't be changed") if city_id_was.present?
+  end
+
+  def referrer_must_exist
+    errors.add(:referrer, "must exist") unless User.exists?(referrer_id)
+  end
+
+  def referrer_cannot_be_self
+    errors.add(:referrer, "can't be you (nice try!)") if referrer == self
   end
 
   def assign_private_leaderboard

spec/presenters/scores/user_scores_presenter_spec.rb

@@ -154,7 +154,8 @@
 
   context "when user has no city" do
     before do
-      user_1.update!(city: nil)
+      user_1.city = nil
+      user_1.save(validate: false)
     end
 
     it "includes no info about it in the output" do
@@ -166,7 +167,8 @@
 
   context "when user has no batch" do
     before do
-      user_1.update!(batch: nil)
+      user_1.batch = nil
+      user_1.save(validate: false)
     end
 
     it "includes no info about it in the output" do