Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
Nov 19, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ec82105
feat(users): validate that referrer exists
wJoenn Nov 12, 2023
3290ed7
feat(users): validate city can't change
wJoenn Nov 12, 2023
5cf2a0a
feat(users): validate batch can't change
wJoenn Nov 12, 2023
3233fff
Merge branch 'main' into feat/users/validation-flashes
wJoenn Nov 18, 2023
b296a59
fix(users): rspec
wJoenn Nov 18, 2023
3eeec6b
fix(users): revert refactos
wJoenn Nov 18, 2023
d24e518
fix(users) disable Metrics/ClassLength for user model
wJoenn Nov 18, 2023
ff2ea49
fix(users): rename cant to cannot
wJoenn Nov 18, 2023
73770ee
fix(users): move referrer param inside params
wJoenn Nov 18, 2023
4663981
feat(users): add explanation for params[:batch_id] = nil if form_para…
wJoenn Nov 18, 2023
541c570
refactor(users): validations params
wJoenn Nov 18, 2023
9f6d5dd
Spacing
pil0u Nov 18, 2023
03e1596
Handle referral_code in a proper update_referrer method (we keep refe…
pil0u Nov 19, 2023
daf8cda
Add back the batch validation
pil0u Nov 19, 2023
c9154b6
Rename update to assign referrer
pil0u Nov 19, 2023
8bba6d3
Wording
pil0u Nov 19, 2023
2d8ec04
Rename, again
pil0u Nov 19, 2023
2cc40d4
Trigger batch/city validations on update
pil0u Nov 19, 2023
6f7ad13
Rollback to the previous behaviour: referrer cannot be nil after it w…
pil0u Nov 19, 2023
ba79750
Test pass, but behaviour is not as expected
pil0u Nov 19, 2023
a065fb6
fix(users): add #to_i to referrer_id to trigger change even if user h…
wJoenn Nov 19, 2023
1f97ead
Explicitly use -1 if referrer is not found
pil0u Nov 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ Lint/MissingSuper:
Lint/SymbolConversion:
EnforcedStyle: consistent

Metrics/ClassLength:
Exclude:
- "**/app/models/user.rb"

Naming/VariableNumber:
EnforcedStyle: snake_case
Exclude:
Expand Down
22 changes: 7 additions & 15 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ def edit
end

def update
if current_user.update(updated_params)
referral_code = params.dig(:user, :referrer_code)
current_user.batch_id = -1 if params.dig(:user, :batch_number) # Set impossible value to trigger validation

if current_user.referrer_valid?(referral_code) && current_user.update(user_params)
unlock_achievements
redirect_back fallback_location: "/", notice: "Your user information was updated"
else
Expand Down Expand Up @@ -85,30 +88,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
50 changes: 40 additions & 10 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ class User < ApplicationRecord
validates :aoc_id, numericality: { in: 1...(2**31), message: "should be between 1 and 2^31" }, allow_nil: true
pil0u marked this conversation as resolved.
Show resolved Hide resolved
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, if: :batch_id_changed?
validate :city_cannot_be_changed_if_present, if: :city_id_changed?
validate :referrer_cannot_be_self

scope :admins, -> { where(uid: ADMINS.values) }
scope :confirmed, -> { where(accepted_coc: true, synced: true).where.not(aoc_id: nil) }
Expand All @@ -39,27 +40,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)
pil0u marked this conversation as resolved.
Show resolved Hide resolved
end

def admin?
Expand Down Expand Up @@ -111,10 +110,41 @@ def referrer_code
referrer&.referral_code
end

def referrer_valid?(referral_code)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

referrer_valid? ne sous entend pas que le referrer sera update.
Pour moi le naming est pas correct ici

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suggère qu'on ne permette pas aux users de reset leur referrer a nil si ils en ont deja un de valide et qu'on remette cette validation comme elle etait avant ton commit 😅

# Do nothing and return true if no referral code is provided
return true if referral_code.nil?

# If the provided referral code is empty, remove the referrer
if referral_code == ""
self.referrer_id = nil
return true
end

# If there is a non-empty referral code, try and find the associated user
referrer = User.find_by_referral_code(referral_code)

# If the user does not exist, add an error and return false
if referrer.nil?
errors.add(:referrer, "must exist")
return false
end

self.referrer_id = referrer.id
true
end

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
pil0u marked this conversation as resolved.
Show resolved Hide resolved

def city_cannot_be_changed_if_present
errors.add(:city, "can't be changed") if city_id_was.present?
end

def referrer_cannot_be_self
errors.add(:referrer, "can't be you (nice try!)") if referrer == self
end

def assign_private_leaderboard
Expand Down
6 changes: 4 additions & 2 deletions spec/presenters/scores/user_scores_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
Loading