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

Plug Batch info on Kitt callback and move back City on User to simplify validation logic. #325

Merged
merged 11 commits into from
Nov 17, 2023
2 changes: 1 addition & 1 deletion app/components/user_form_component.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

<div class="flex flex-col gap-y-1 sm:flex-row sm:items-center sm:gap-x-3">
<%= f.label :campus_name, "Campus" %>
<%= f.collection_select(:city_id, City.order(:vanity_name), :id, :vanity_name, { prompt: "", selected: @user.batch&.city_id }, class: "input", disabled: @user.batch&.city_id?) %>
<%= f.collection_select(:city_id, City.order(:vanity_name), :id, :vanity_name, { prompt: "", selected: @user.city_id }, class: "input", disabled: @user.city_id?) %>
</div>

<div class="flex items-center gap-x-3 w-48 sm:w-fit">
Expand Down
8 changes: 1 addition & 7 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,12 @@ def restrict_after_lock
end

def updated_params
batch = nil

if current_user.batch_id.nil? && form_params[:city_id] # rubocop:disable Style/IfUnlessModifier
batch = Batch.find_or_create_by(number: nil, city_id: form_params[:city_id])
end

{
accepted_coc: form_params[:accepted_coc],
aoc_id: form_params[:aoc_id],
entered_hardcore: form_params[:entered_hardcore],
username: form_params[:username],
batch_id: batch&.id,
city_id: form_params[:city_id],
referrer: User.find_by_referral_code(form_params[:referrer_code])
}.compact
end
Expand Down
2 changes: 1 addition & 1 deletion app/domains/scores/city_points.rb
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

J'ai complètement oublié de passer sur les computers. Bien vu.

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def compute
points = Scores::SoloPoints.get

# index for o(1) fetch
city_for_user = User.joins(:batch).where(id: points.pluck(:user_id)).pluck(:id, "batches.city_id").to_h
city_for_user = User.where(id: points.pluck(:user_id)).pluck(:id, :city_id).to_h
city_users = points.group_by { |user| city_for_user[user[:user_id]] }

city_users.without(nil).flat_map do |city_id, user_points|
Expand Down
80 changes: 0 additions & 80 deletions app/jobs/kitt_scraper_job.rb

This file was deleted.

1 change: 0 additions & 1 deletion app/models/batch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

class Batch < ApplicationRecord
has_many :users, dependent: :nullify
belongs_to :city

validates :number, numericality: { in: 1...(2**31), message: "should be between 1 and 2^31" }, allow_nil: true
end
5 changes: 2 additions & 3 deletions app/models/city.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ class City < ApplicationRecord
has_many :city_points, class_name: "Cache::CityPoint", dependent: :delete_all
has_many :city_scores, class_name: "Cache::CityScore", dependent: :delete_all

has_many :batches, dependent: :nullify
has_many :users, through: :batches
has_many :users, dependent: :nullify
has_many :completions, through: :users

before_create :set_default_vanity_name

validates :name, uniqueness: { case_sensitive: false }
validates :name, presence: true, uniqueness: { case_sensitive: false }

def self.find_by_slug(slug)
find_by!("REPLACE(LOWER(name), ' ', '-') = ?", slug)
Expand Down
17 changes: 11 additions & 6 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ class User < ApplicationRecord
CONTRIBUTORS = { pilou: "6788", aquaj: "449", louis: "19049", aurelie: "9168" }.freeze

belongs_to :batch, optional: true
belongs_to :city, optional: true, touch: true
belongs_to :squad, optional: true, touch: true
belongs_to :referrer, class_name: "User", optional: true

delegate :city, :city_id, to: :batch, allow_nil: true

has_many :completions, dependent: :destroy
has_many :user_day_scores, class_name: "Cache::UserDayScore", dependent: :delete_all
has_many :solo_points, class_name: "Cache::SoloPoint", dependent: :delete_all
Expand All @@ -38,13 +37,19 @@ class User < ApplicationRecord
after_create :assign_private_leaderboard

def self.from_kitt(auth)
user = where(provider: auth.provider, uid: auth.uid).first_or_create do |u|
batches = auth.info&.schoolings
oldest_batch = batches.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.github_username = auth.info.github_nickname
u.batch_id = Batch.find_or_create_by(number: auth.info.last_batch_slug.to_i).id
u.batch = Batch.find_or_initialize_by(number: oldest_batch&.camp&.slug.to_i)
Copy link
Owner

@pil0u pil0u Nov 16, 2023

Choose a reason for hiding this comment

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

Est-ce que ça veut dire qu'on crée un batch 0 quoi qu'il arrive ? C'est mieux un batch 0 ou un batch nil ? cc @Aquaj

Edit : à choisir, nil ça évite d'afficher un vieux 0 tout moche dans le front

Copy link
Collaborator

Choose a reason for hiding this comment

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

Non

validates :number, numericality: { in: 1...(2**31), message: "should be between 1 and 2^31" }, allow_nil: true

Copy link
Owner

Choose a reason for hiding this comment

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

Sauf que nil.to_i == 0, il faudrait échapper avec .& non ?

Copy link
Collaborator

@wJoenn wJoenn Nov 16, 2023

Choose a reason for hiding this comment

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

La validation du Batch ne permet pas de creer un batch avec un number 0
Donc nil.to_i ne respectera pas la validation et le batch ne sera pas persisté du coup le batch sera nil

u.city = City.find_or_initialize_by(name: oldest_batch&.city&.name)
end

user.update(github_username: auth.info.github_nickname)
user.github_username = auth.info.github_nickname

user.save

user
end

Expand Down
2 changes: 1 addition & 1 deletion app/presenters/scores/user_scores_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def initialize(scores)
def get
@scores ||= User # rubocop:disable Naming/MemoizedInstanceVariableName
.where(id: scores_per_user.keys)
.includes(:squad, :completions, batch: :city)
.includes(:squad, :completions, :city)
.map { |user| { **identity_of(user), **stats_of(user) } }
.sort_by { |user| user[:order] || Float::INFINITY }
end
Expand Down
10 changes: 10 additions & 0 deletions db/migrate/20231116030303_add_back_city_on_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

class AddBackCityOnUsers < ActiveRecord::Migration[7.1]
disable_ddl_transaction!

def change
add_reference :users, :city, index: { algorithm: :concurrently }
add_foreign_key :users, :cities, validate: false
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddForeignKeyForUsersCityRelationship < ActiveRecord::Migration[7.1]
def change
validate_foreign_key :users, :cities
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true

class RemoveCityAndSizeFromBatches < ActiveRecord::Migration[7.1]
def change
safety_assured { remove_column :batches, :city_id, :bigint }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Non on en a besoin. C'est elle qui force l'User à s'aligner sur son batch.

Copy link
Collaborator

@wJoenn wJoenn Nov 16, 2023

Choose a reason for hiding this comment

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

Y a pas de choix, donc y a pas besoin de forcer.
Un user qui a un batch se connecte ?
alors le User model recupere le batch et la city. KittAuth peu pas se tromper, notre data viens du meme endroit donc si ils ont tord, on a tord.
Et comme il a deja les deux il pourras jamais changer.

Un user a pas de batch ?
Alors on a rien a verifier

Copy link
Collaborator Author

@Aquaj Aquaj Nov 17, 2023

Choose a reason for hiding this comment

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

Je pense qu'il y a eu quiproquo: quand je dis "C'est elle qui force l'User à s'aligner sur son batch." ce que je veux dire c'est que c'est elle qui force l'User à s'aligner sur son batch dans notre modèle de donnée. Ça rend explicite une relation qui est seulement implicite ici.

"Y'a pas de choix donc pas besoin de forcer" c'est pas un bon argument. On est dans la couche de cohérence des données là, la présence ou absence de choix est dans la couche applicative.

C'est pas la mort de l'avoir enlevée, mais je trouve ça dommage.

Copy link
Owner

Choose a reason for hiding this comment

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

On pourra peut-être rétablir 🫣

safety_assured { remove_column :batches, :size, :int }
end
end
9 changes: 4 additions & 5 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.1].define(version: 2023_11_16_024537) do
ActiveRecord::Schema[7.1].define(version: 2023_11_16_174333) do
# These are extensions that must be enabled in order to support this database
enable_extension "citext"
enable_extension "pgcrypto"
Expand All @@ -28,12 +28,9 @@
end

create_table "batches", force: :cascade do |t|
t.bigint "city_id"
t.datetime "created_at", null: false
t.integer "number"
t.integer "size"
t.datetime "updated_at", null: false
t.index ["city_id"], name: "index_batches_on_city_id"
end

create_table "blazer_audits", force: :cascade do |t|
Expand Down Expand Up @@ -374,6 +371,7 @@
t.boolean "accepted_coc", default: false, null: false
t.integer "aoc_id"
t.bigint "batch_id"
t.bigint "city_id"
t.datetime "created_at", null: false
t.boolean "entered_hardcore", default: false, null: false
t.string "github_username"
Expand All @@ -392,11 +390,11 @@
t.string "username"
t.index ["aoc_id"], name: "index_users_on_aoc_id", unique: true
t.index ["batch_id"], name: "index_users_on_batch_id"
t.index ["city_id"], name: "index_users_on_city_id"
t.index ["referrer_id"], name: "index_users_on_referrer_id"
end

add_foreign_key "achievements", "users"
add_foreign_key "batches", "cities"
add_foreign_key "city_points", "cities"
add_foreign_key "city_scores", "cities"
add_foreign_key "completions", "users"
Expand All @@ -409,5 +407,6 @@
add_foreign_key "squad_points", "squads"
add_foreign_key "squad_scores", "squads"
add_foreign_key "users", "batches"
add_foreign_key "users", "cities"
add_foreign_key "users", "users", column: "referrer_id"
end
Loading