Plug Batch info on Kitt callback and move back City on User to simplify validation logic. #325
Conversation
…fy validation logic.
| <div class="flex flex-col gap-y-1 sm:flex-row sm:items-center sm:gap-x-3"> | ||
| <%= f.label :batch_number, "Batch #" %> | ||
| <%= f.number_field :batch_number, value: @user.batch&.number, min: 1, class: "input", disabled: true %> | ||
| <%= f.number_field :batch_number, value: @user.batch&.number, min: 1, class: "input", disabled: @user.batch_id? %> | ||
| </div> |
There was a problem hiding this comment.
Le batch devrait toujours etre disabled.
Soit les gens en ont un et ils peuvent pas changer
Soit ils en ont pas et ils peuvent pas s'en rajouter un.
There was a problem hiding this comment.
Soit ils en ont pas et ils peuvent pas s'en rajouter un.
J'avais pas cette info 😅 Je savais pas qu'on pouvait être "sans-batch", @pil0u on gère bien leurs points à eux ? Je me souviens plus de l'an dernier.
There was a problem hiding this comment.
@Aquaj Oui, on peut récupérer un batch 0 ou nil sur l'OAuth, des profs extérieurs par exemple, j'évalue à ~1000 Alumnis concernés quand j'ai lancé le scraper, donc pas négligeable.
Mais oui, on n'a plus de scoring par batch, uniquement par squad et campus. Et pour le campus, comme ils choisissent eux-mêmes parmi une liste existante, ils sont pris en compte 👍
app/controllers/users_controller.rb
Outdated
| { | ||
| 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, | ||
| batch_id: Batch.find_by(number: form_params[:batch_number])&.id, | ||
| city_id: form_params[:city_id], | ||
| referrer: User.find_by_referral_code(form_params[:referrer_code]) | ||
| }.compact |
There was a problem hiding this comment.
Same as previous comment, the batch should never be updated by the user event if he has none.
app/models/user.rb
Outdated
| def city_must_match_batch | ||
| return unless batch&.city_id? | ||
|
|
||
| errors.add(:city_id, "must match batch city") if city_id != batch.city_id | ||
| end |
There was a problem hiding this comment.
Qu'ets ce qui se passe si tu as une city mais pas de batch ?
There was a problem hiding this comment.
Le return unless batch&.city_id? ligne 124 fera que la validation sera juste pas appliquée.
There was a problem hiding this comment.
Sauf que tu as changé le model du Batch en disant que la city est optional.
Ca veut dire que tu pourrais avoir un batch avec un city_id nil qui casserait ensuite la validation non ?
There was a problem hiding this comment.
Pour ça que le return checke aussi la présence de la city sur le Batch. Si le batch n'a pas de city_id on skippe aussi la validation.
C'est obsolète par contre si on permet pas de choisir son batch. L'optional sur Batch#city saute et on redeviendra juste return unless batch.present? ici pour la simplicité.
app/models/batch.rb
Outdated
| class Batch < ApplicationRecord | ||
| has_many :users, dependent: :nullify | ||
| belongs_to :city | ||
| belongs_to :city, optional: true | ||
|
|
||
| validates :number, numericality: { in: 1...(2**31), message: "should be between 1 and 2^31" }, allow_nil: true | ||
| end |
There was a problem hiding this comment.
Si on garde une reference a city sur le batch alors je comprend pas comment un batch pourrait se retrouver sans city
There was a problem hiding this comment.
C'était surtout pour le système de choix de batch en cas de batch manquant sur Kitt. (Pcq du coup l'user choisirait un batch au choix, potentiellement un qu'on connaît pas et donc qui a pas de ville)
Mais si c'est pas nécessaire, je vais faire sauter ça.
|
@Aquaj J'ai pas de commentaire sur le code en soi. Je me permets de faire une synthèse des specs parce que je crois que j'ai écrit ça nulle part 👀
|
|
|
||
| class RemoveCityAndSizeFromBatches < ActiveRecord::Migration[7.1] | ||
| def change | ||
| safety_assured { remove_column :batches, :city_id, :bigint } |
There was a problem hiding this comment.
Non on en a besoin. C'est elle qui force l'User à s'aligner sur son batch.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
app/domains/scores/city_points.rb
Outdated
There was a problem hiding this comment.
J'ai complètement oublié de passer sur les computers. Bien vu.
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Non
validates :number, numericality: { in: 1...(2**31), message: "should be between 1 and 2^31" }, allow_nil: trueThere was a problem hiding this comment.
Sauf que nil.to_i == 0, il faudrait échapper avec .& non ?
There was a problem hiding this comment.
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
db/seeds.rb
Outdated
| { name: "Toulouse", size: 20 }, | ||
| { name: "Tunis", size: 0 }, | ||
| { name: "Zurich", size: 64 }, | ||
| { name: "remote", size: 0 } |
There was a problem hiding this comment.
Désolé, j'avais pas vu que dans ma liste il y avait des 0... tu peux les enlever (je le ferai si t'es capot)
There was a problem hiding this comment.
Y avait pas de 0 dans ta liste.
J'ai fais City.all.map.sort_by en pensant que ca serait le meme resultat pour avoir une hash directement
…fy validation logic. (#325) * Plug Batch info on Kitt callback and move back City on User to simplify validation logic. * Rubocop * fix(users): revert some changes * fix(users): undo everything from #289 * fix(users): use kitt city name instead of slug * fix(users) last comments * fix(users): remove empty city froms seed * Wording + spacing * Remove Testville --------- Co-authored-by: Louis Ramos <louisramosdev@gmail.com> Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com>
* Update README.md (#305) * Update README.md * Update README.md * Update README.md * Spacing * feat(users): Integrate a mapping between batch numbers and campuses from scrapped data (#289) * Migrate fork branch * batch_map.csv added to assets * removed size from City model * added size to Batch model * added city reference to Batch * added association between Batch and City + removed size from City model * Added batch_map to seed + gave uid to test users * added automatic city_id distribution on save to User model * added some changes by Aquaj to seed.rb and users/edit.erb * refactor * Removed batch from permitted parameters * Removed city foreign_key from users * Removed ignored_columns size from city and city_id from user * Moved batch_map.csv to db/static * Add kitt scrapper script (python) to db/scripts * fix(cities): restore size column for cities table * feat(batch_map): rewrite scrapper in ruby and change data to be user scoped including batch year * fix(batch_map): refactor seed to avoid doing multiple request to the DB for the same City * feat(batch_map): move scrapper to a Job with CRON on Dec 1st * Disabled city update for users that have a batch * fix(batch_map): rubocop offense * Rename batch_map csv to kill_alumni csv * Remove batch_year column migration * Refactor current_user.batch to explicit #blank? and #present? booleans * Add vanity_name column to cities * Fix typo in Scraper * Rename KittScraperJob kwargs * Refactor KittScrapperJob with CSV headers * Fix users without batches not able to choose a city since remove city from User * Fix tests * Readd disabled confitions in UserFormComponent * fix(users): moved user city validation inside model * Fix CityPoints spec * Fix CityScores spec * Fix spacing * Clarify condition on disabled city input * Remove cron for Kitt scraper * Set vanity name before create * Handle case when user has no batch and picks a city * Fix display of scraper pages --------- Co-authored-by: Louis Ramos <wjoenn@gmail.com> Co-authored-by: Jérémie <jeremie.bonal@gmail.com> Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Bump sentry-rails and sentry-ruby (#309) Bumps [sentry-rails](https://github.com/getsentry/sentry-ruby) and [sentry-ruby](https://github.com/getsentry/sentry-ruby). These dependencies needed to be updated together. Updates `sentry-rails` from 5.12.0 to 5.13.0 - [Release notes](https://github.com/getsentry/sentry-ruby/releases) - [Changelog](https://github.com/getsentry/sentry-ruby/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-ruby@5.12.0...5.13.0) Updates `sentry-ruby` from 5.12.0 to 5.13.0 - [Release notes](https://github.com/getsentry/sentry-ruby/releases) - [Changelog](https://github.com/getsentry/sentry-ruby/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-ruby@5.12.0...5.13.0) --- updated-dependencies: - dependency-name: sentry-rails dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: sentry-ruby dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Double quotes everywhere (#311) * Double quotes everywhere * Encore * Re-add the staging environment in Sentry (#313) * Fix GithubStar achievement by doing nothing if the response code of the request is not 200 (#310) * fix(users) rename leftover 'city' to 'campus' (#315) * feat(users): enabled cookie based referral system (#291) * Add #referrer and #referrees to User * Add referrer code form to setup * Add referrement code to settings page * fix erblint * fix(users) minor templating changes * fix(users): refactored User::by_referral_code * fix(users): disable self-referral * fix(users): add find_by_referral_code to Rubocop::Rails::DynamicFindBy::Whitelist * fix(users): referrer from ternary to ... unless ... * feat(users): enabled cookie based referral system * fix(users): apply review comments * feat(users): display referrees in settings (#314) * feat(users): update referral sentence in settings * feat(users): add links to referrees in settings * feat(users): add links for squad users * refactor(users): remove #to_raw_links * refactor(users): sanitize to safe_join * Add margin and pluralize the user(s) * Rename referree to referee * Add instance variable, for readability + fix quotes --------- Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Auto-assign a leaderboard to new users (#286) * Add Louis and Aurélie as contributors * Add private_leaderboard column to Users * Assign the emptiest leaderboard to every new user * Add AOC_ROOMS env variable in tests to allow user update * Move the AOC_ROOMS env variable for the test environment into a dedicated .env.test file * Naming * Add guard clause if for some reason the private leaderboard is already associated to the user * Create an Admin page, with links to useful tools (#312) * Add an impersonate User action for admins * Add an admin page from which you can login, but accessible only for admins. It contains link to admin tools: blazer, good_job and the impersonation tool * Add useful heroku commands * Add the possibility to impersonate via uid or aoc_id * Spacing * Spacing * Slightly adjust finding logic + add false-positive to brakeman ignore file * Rollback formatting --------- Co-authored-by: Jérémie <jeremie.bonal@gmail.com> * Update the Code of Conduct (#321) * Rename to code of conduct with all letters in the footer * Improved the wording, emphasised the importance of being respectful, friendly and positive * Bump good_job from 3.21.0 to 3.21.1 (#323) Bumps [good_job](https://github.com/bensheldon/good_job) from 3.21.0 to 3.21.1. - [Release notes](https://github.com/bensheldon/good_job/releases) - [Changelog](https://github.com/bensheldon/good_job/blob/main/CHANGELOG.md) - [Commits](bensheldon/good_job@v3.21.0...v3.21.1) --- updated-dependencies: - dependency-name: good_job dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Slack account link (#316) * Better handling of failures during omniauth signin Fix nesting so custom defined `failure` actually exists on the controller + rely on Devise `failure_message` which is better implemented than our own error-message extractor + extract part of thelogic to small private method that'll help us later * Add Slack OpenID Omniauth strategy * Add button to link account * Add unlinking mechanism * Rubocop * Add spec for unlinking * Add back controllers: config bc it's actually needed to set some internal attributes of OmniAuth * Create helper to handle complex error-mocking logic for OmniAuth * Add spec for omniauth slack callback * Rubocop * ERB Lint * Rubocop Again * Consistent notice before redirect * Spacing * Reorder methods and add slack_deep_link * Add Font Awesome Slack icon (and remove the unused Plus) * Remove margin from component * Adjust style for the Slack integration section * Slightly adjust the spacing in squad settings * Move the Slack settings below the Squad settings * Add handling of failed updates * Add storage for Slack access-token as we might need it. * Use fail_auth in #kitt Omniauth callback too. * Rubocop * Add Slack credentials and active record encryption key set in production --------- Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * fix(campus): rename view files and template references of city to campus (#324) * fix(campus): rename view files and template references of city to campus * fix(campus): tests * feat(campus): further migrate cities to campuses * fix(campus): update campus route path * Fix the user show * Show and order cities dropdown by vanity_name * Form label * Update campus show * CityRowComponent -> CampusRowComponent --------- Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Plug Batch info on Kitt callback and move back City on User to simplify validation logic. (#325) * Plug Batch info on Kitt callback and move back City on User to simplify validation logic. * Rubocop * fix(users): revert some changes * fix(users): undo everything from #289 * fix(users): use kitt city name instead of slug * fix(users) last comments * fix(users): remove empty city froms seed * Wording + spacing * Remove Testville --------- Co-authored-by: Louis Ramos <louisramosdev@gmail.com> Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Refresh the FAQ (#326) * Update the wording of the existing FAQ * Remove data notice in settings, it's already in the FAQ * Reorder questions * Remove horrendous spaces * Remove CSV --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Louis Ramos <75388869+wJoenn@users.noreply.github.com> Co-authored-by: Louis Ramos <wjoenn@gmail.com> Co-authored-by: Jérémie <jeremie.bonal@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Louis Ramos <louisramosdev@gmail.com>
* Update README.md (#305) * Update README.md * Update README.md * Update README.md * Spacing * feat(users): Integrate a mapping between batch numbers and campuses from scrapped data (#289) * Migrate fork branch * batch_map.csv added to assets * removed size from City model * added size to Batch model * added city reference to Batch * added association between Batch and City + removed size from City model * Added batch_map to seed + gave uid to test users * added automatic city_id distribution on save to User model * added some changes by Aquaj to seed.rb and users/edit.erb * refactor * Removed batch from permitted parameters * Removed city foreign_key from users * Removed ignored_columns size from city and city_id from user * Moved batch_map.csv to db/static * Add kitt scrapper script (python) to db/scripts * fix(cities): restore size column for cities table * feat(batch_map): rewrite scrapper in ruby and change data to be user scoped including batch year * fix(batch_map): refactor seed to avoid doing multiple request to the DB for the same City * feat(batch_map): move scrapper to a Job with CRON on Dec 1st * Disabled city update for users that have a batch * fix(batch_map): rubocop offense * Rename batch_map csv to kill_alumni csv * Remove batch_year column migration * Refactor current_user.batch to explicit #blank? and #present? booleans * Add vanity_name column to cities * Fix typo in Scraper * Rename KittScraperJob kwargs * Refactor KittScrapperJob with CSV headers * Fix users without batches not able to choose a city since remove city from User * Fix tests * Readd disabled confitions in UserFormComponent * fix(users): moved user city validation inside model * Fix CityPoints spec * Fix CityScores spec * Fix spacing * Clarify condition on disabled city input * Remove cron for Kitt scraper * Set vanity name before create * Handle case when user has no batch and picks a city * Fix display of scraper pages --------- Co-authored-by: Louis Ramos <wjoenn@gmail.com> Co-authored-by: Jérémie <jeremie.bonal@gmail.com> Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Bump sentry-rails and sentry-ruby (#309) Bumps [sentry-rails](https://github.com/getsentry/sentry-ruby) and [sentry-ruby](https://github.com/getsentry/sentry-ruby). These dependencies needed to be updated together. Updates `sentry-rails` from 5.12.0 to 5.13.0 - [Release notes](https://github.com/getsentry/sentry-ruby/releases) - [Changelog](https://github.com/getsentry/sentry-ruby/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-ruby@5.12.0...5.13.0) Updates `sentry-ruby` from 5.12.0 to 5.13.0 - [Release notes](https://github.com/getsentry/sentry-ruby/releases) - [Changelog](https://github.com/getsentry/sentry-ruby/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-ruby@5.12.0...5.13.0) --- updated-dependencies: - dependency-name: sentry-rails dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: sentry-ruby dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Double quotes everywhere (#311) * Double quotes everywhere * Encore * Re-add the staging environment in Sentry (#313) * Fix GithubStar achievement by doing nothing if the response code of the request is not 200 (#310) * fix(users) rename leftover 'city' to 'campus' (#315) * feat(users): enabled cookie based referral system (#291) * Add #referrer and #referrees to User * Add referrer code form to setup * Add referrement code to settings page * fix erblint * fix(users) minor templating changes * fix(users): refactored User::by_referral_code * fix(users): disable self-referral * fix(users): add find_by_referral_code to Rubocop::Rails::DynamicFindBy::Whitelist * fix(users): referrer from ternary to ... unless ... * feat(users): enabled cookie based referral system * fix(users): apply review comments * feat(users): display referrees in settings (#314) * feat(users): update referral sentence in settings * feat(users): add links to referrees in settings * feat(users): add links for squad users * refactor(users): remove #to_raw_links * refactor(users): sanitize to safe_join * Add margin and pluralize the user(s) * Rename referree to referee * Add instance variable, for readability + fix quotes --------- Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Auto-assign a leaderboard to new users (#286) * Add Louis and Aurélie as contributors * Add private_leaderboard column to Users * Assign the emptiest leaderboard to every new user * Add AOC_ROOMS env variable in tests to allow user update * Move the AOC_ROOMS env variable for the test environment into a dedicated .env.test file * Naming * Add guard clause if for some reason the private leaderboard is already associated to the user * Create an Admin page, with links to useful tools (#312) * Add an impersonate User action for admins * Add an admin page from which you can login, but accessible only for admins. It contains link to admin tools: blazer, good_job and the impersonation tool * Add useful heroku commands * Add the possibility to impersonate via uid or aoc_id * Spacing * Spacing * Slightly adjust finding logic + add false-positive to brakeman ignore file * Rollback formatting --------- Co-authored-by: Jérémie <jeremie.bonal@gmail.com> * Update the Code of Conduct (#321) * Rename to code of conduct with all letters in the footer * Improved the wording, emphasised the importance of being respectful, friendly and positive * Bump good_job from 3.21.0 to 3.21.1 (#323) Bumps [good_job](https://github.com/bensheldon/good_job) from 3.21.0 to 3.21.1. - [Release notes](https://github.com/bensheldon/good_job/releases) - [Changelog](https://github.com/bensheldon/good_job/blob/main/CHANGELOG.md) - [Commits](bensheldon/good_job@v3.21.0...v3.21.1) --- updated-dependencies: - dependency-name: good_job dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Slack account link (#316) * Better handling of failures during omniauth signin Fix nesting so custom defined `failure` actually exists on the controller + rely on Devise `failure_message` which is better implemented than our own error-message extractor + extract part of thelogic to small private method that'll help us later * Add Slack OpenID Omniauth strategy * Add button to link account * Add unlinking mechanism * Rubocop * Add spec for unlinking * Add back controllers: config bc it's actually needed to set some internal attributes of OmniAuth * Create helper to handle complex error-mocking logic for OmniAuth * Add spec for omniauth slack callback * Rubocop * ERB Lint * Rubocop Again * Consistent notice before redirect * Spacing * Reorder methods and add slack_deep_link * Add Font Awesome Slack icon (and remove the unused Plus) * Remove margin from component * Adjust style for the Slack integration section * Slightly adjust the spacing in squad settings * Move the Slack settings below the Squad settings * Add handling of failed updates * Add storage for Slack access-token as we might need it. * Use fail_auth in #kitt Omniauth callback too. * Rubocop * Add Slack credentials and active record encryption key set in production --------- Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * fix(campus): rename view files and template references of city to campus (#324) * fix(campus): rename view files and template references of city to campus * fix(campus): tests * feat(campus): further migrate cities to campuses * fix(campus): update campus route path * Fix the user show * Show and order cities dropdown by vanity_name * Form label * Update campus show * CityRowComponent -> CampusRowComponent --------- Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Plug Batch info on Kitt callback and move back City on User to simplify validation logic. (#325) * Plug Batch info on Kitt callback and move back City on User to simplify validation logic. * Rubocop * fix(users): revert some changes * fix(users): undo everything from #289 * fix(users): use kitt city name instead of slug * fix(users) last comments * fix(users): remove empty city froms seed * Wording + spacing * Remove Testville --------- Co-authored-by: Louis Ramos <louisramosdev@gmail.com> Co-authored-by: pil0u <8350914+pil0u@users.noreply.github.com> * Refresh the FAQ (#326) * Update the wording of the existing FAQ * Remove data notice in settings, it's already in the FAQ * Reorder questions * Remove horrendous spaces * Remove CSV --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Louis Ramos <75388869+wJoenn@users.noreply.github.com> Co-authored-by: Louis Ramos <wjoenn@gmail.com> Co-authored-by: Jérémie <jeremie.bonal@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Louis Ramos <louisramosdev@gmail.com>
Summary of changes and context
closes #306
Rework structure of User/Batch/City to better use new Kitt OAuth
schoolingsinfo.Sanity checks