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

[wJoenn]

Summary of changes and context

closes #307

"Batch or City can't be changed" if user tries to edit their batch_number
"Batch or City can't be changed" if user has a batch and tries to edit their city_id
"Referrer must exist" if user inputs a incorrect referral_code

Sanity checks

• Linters pass
• Tests pass
• Related GitHub issues are linked in the description

[pil0u]

@wJoenn Un petit rebase de main ? Je pense que la PR va se simplifier :D

[wJoenn]

@pil0u Le model de User commence faire plus de 100 lignes ce qui trigger Rubocop.
Du coup j'en ai supprimé quelques une en faisant un léger refacto

[pil0u]

Si Rubocop est relou avec la longueur de fichier, soit on ajuste Rubocop pour qu'il nous laisse tranquille, soit on bouge des bouts de code ailleurs, mais partir sur des one-liners n'est pas la bonne approche. On ne rend pas le code moins lisible (voire changer la logique) pour plaire au linter 😅

[wJoenn]

Je trouve pas qu'il y ai le moindre refacto que j'ai fais ici qui rend le code moins lisible mais si tu veux.

Du coup tu veux faire quoi par rapport a ca ?

Augmenter la limite ? Le desactivé ? Ou casser le User model en morceau ?

[pil0u]

@wJoenn Pour RuboCop, tu peux ajouter une exception pour cette règle, pour ce fichier là 👍

[wJoenn]

C'est un peu confu les validations donc je vais essayer de reprendre ca point par point.

Batch

• On veut le valider donc on permit le param

  def updated_params
    params = {
      batch: Batch.find_by(number: form_params[:batch_number])
    }.compact

    params
  end
  
  def form_params
    params.require(:user).permit(:batch_number)
  end

• On veut un message d'erreur meme si le batch n'existe pas donc on renvoit toujours une valeur.

  def updated_params
    params = {
      batch_id: Batch.find_by(number: form_params[:batch_number])&.id.to_i
    }.compact

    params
  end

• On ne veut pas qu'il bloque toutes les update par le fait qu'il aura toujours une valeur de 0 quand le param est nil donc on rajoute une condition

  def updated_params
    params = {
      batch_id: (Batch.find_by(number: form_params[:batch_number])&.id.to_i if form_params[:batch_number].present?)
    }.compact

    params
  end

• On ne veut pas qu'il puisse changer donc on l'interdit dans la validation

  def batch_cannot_change
    errors.add(:batch, "can't be changed") if batch_changed?
  end

• Il doit pouvoir changer une fois, lors de la création du User, donc on verifie aussi qu'il existait deja au moment du changement

  def batch_cannot_change
    errors.add(:batch, "can't be changed") if batch_changed? && batch_id_was.present?
  end

• Maintenant qu'on ne bloque plus la validation dans le cas ou il n'y avait pas de batch, on se trouve fasse a un probleme concernant les User qui n'ont pas de batch du tout qui peuvent donc le modifier une fois. Pour eviter cela on utilise toujours nil comme valeur lors de l'update. Et comme nil serait enlever lors du compact on le deplace en dehors

  def updated_params
    params = {
    }.compact
    
     params[:batch] = nil if form_params[:batch_number].present?

    params
  end

Referrer

• On veut le valider donc on permit les params

  def updated_params
    params = {
      referrer: User.find_by_referral_code(form_params[:referrer_code])
    }.compact

    params
  end
  
  def form_params
    params.require(:user).permit(:referrer_code)
  end

• On veut valider que le referrer existe donc on utilise l'id pour toujours avoir 0

```ruby
  def updated_params
    params = {
      referrer_id: User.find_by_referral_code(form_params[:referrer_code])&.id.to_i
    }.compact

    params
  end

  def referrer_exists
    errors.add(:referrer, "must exist") if referrer_id == 0
  end

• On ne veut pas que ca bloque les autre validations donc on ajoute une condition

  def updated_params
    params = {
      referrer_id: (User.find_by_referral_code(form_params[:referrer_code])&.id.to_i if form_params[:referrer_code].present?)
    }.compact

    params
  end

[pil0u]

@wJoenn J'ai testé le dernier code que tu as push, malheureusement il ne fonctionnait pas tout à fait - j'obtenais une erreur sur le batch quand j'appuyais sur "SAVE", le referral code s'enlevait pas si on laissait vide après avoir entré un code valide...

Beaucoup de discussions et d'aller-retour pour un truc si micro 🥵 Le bilan :

• on a simplifié les strong params en dégageant ce updated_params un peu bâtard
• on empêche correctement de mettre à jour le campus une fois qu'il est set
• avec update_referrer, on a le comportement qui ne bouge, avec en plus un message d'erreur dans le cas où le parrain n'est pas trouvé
• on abandonne l'alerte sur le changement de batch, c'est faisable (j'ai le code dans un coin) mais c'est moche et pas satisfaisant - tant pis

[wJoenn]

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

[pil0u]

Suggestions ?

[wJoenn]

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 😅

[wJoenn]

@pil0u by doing this you end up with a id of nil if a User is not found

 current_user.referrer_id = User.find_by_referral_code(referrer_code)&.id if referrer_code

If a User has no referrer yet it means its referrer_id is nil.
When you try add one and you use an incorrect referral code you end up trying to update the referrer_id from nil to nil
This does not trigger the if: :referrer_id_changed? condition so the validation is skipped.

By using this instead

 current_user.referrer_id = User.find_by_referral_code(referrer_code)&.id.to_i if referrer_code

We update from nil to 0 which is a change and it'll trigger the validation that ends up failing and we have the error message that we wanted

[pil0u]

@wJoenn La conversion en .to_i est trop cryptique, j'ai utilisé -1 comme au-dessus.

Une solution alternative que je venais de terminer, c'est ça :

# User model

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

    if referral_code == ""
      errors.add(:referrer, "can't be blank")
      return false
    end

    referrer = User.find_by_referral_code(referral_code)

    if referrer.nil? # not found
      errors.add(:referrer, "must exist")
      return false
    end

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

    self.referrer_id = referrer.id
    true
  end

# user controller


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

    if is_referrer_valid && current_user.update(user_params)
      unlock_achievements
      redirect_back fallback_location: "/", notice: "Your user information was updated"
    else
      redirect_back fallback_location: "/", alert: current_user.errors.full_messages[0].to_s
    end
  end

En gros je créais une méthode d'instance qui gère l'assignation et la validation du referrer. J'en avais profité pour y intégrer toutes les validations.

Je mets ça pour la postérité, si @Aquaj passe par là il aura sûrement un œil d'expert que je n'ai pas sur ce sujet

[wJoenn]

Je trouve ca bizarre de faire une méthode qui s'occupe de faire une assignation, une validation et une confirmation en meme temps perso.
Je suis plus habitué a voir la structure actuelle

[Aquaj]

Suggested change

	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)
	referrer_code = params.dig(:user, :referrer_code)
	if referrer_code
	current_user.referrer_id = User.find_by_referral_code(referrer_code)&.id
	end
	if current_user.update(user_params)

On a pas besoin de ces histoires de -1 avec les validations dans User (faudrait juste rajouter referral_code) il me semble. Quelque chose m'échappe ?

[wJoenn]

Si un user ouvre le DevTool et enleve manuellement le disable du bouton pour le batch, rentre la valeur qu'il veut et submit, on veut quand meme pouvoir lui mettre un message d'erreur.
D'ou

current_user.batch_id = -1 if params.dig(:user, :batch_number)

Et si un user veut changer son referrer pendant le setup il a besoin de pouvoir rentré un referral_code, d'ou

    referrer_code = params.dig(:user, :referrer_code)
    current_user.referrer_id = User.find_by_referral_code(referrer_code)&.id if referrer_code

Sauf que si il rentre un code erroné on veut quand meme pouvoir validé, donc on ajoute || -1 si le code est erroné

[Aquaj]

Si un user ouvre le DevTool et enleve manuellement le disable du bouton pour le batch, rentre la valeur qu'il veut et submit, on veut quand meme pouvoir lui mettre un message d'erreur.

Il se ferait tej par cette validation normalement.

Sauf que si il rentre un code erroné on veut quand meme pouvoir validé, donc on ajoute || -1 si le code est erroné

Ça en effet, mais on pourrait mieux le gérer via un attribute :referrer_code qui n'existe pas en DB mais sert à ce genre de validations.

[wJoenn]

Il se ferait tej par cette validation normalement.

Non parce que le param est pas permitted.
On trouvais pas ca logiquer de permit un param qu'on va jamais autoriser alors plutot que de l'ajouter dans le require.permit on s'est dit que ca faisait plus de sens de le mettre dans l'update .
Et du coup quitte a l'assigner a qqch Pilou preferait mettre -1 pour que ca soit bien clair que c'etait impossible

[wJoenn]

on pourrait mieux le gérer via un attribute :referrer_code

pas la ref

[pil0u]

C'est pas beau, j'ai pas trouvé mieux dans le temps imparti 👀

[Aquaj]

Pas de soucis. Je verrai bien plus tard si on peut clean ça.