[Résolu][Rails] Gestion des erreurs dans formulaire avec un objet non-ActiveRecord

[Maz]

Salut!
Petit soucis, j'ai besoin de généré un formulaire avec un seul champs "quantité", qui enverras sur une fonction create d'un activerecord qui généreras tous paramètre de ce derniers.

J'ai donc simplement fait mon formulaire:

<%= form_tag('user/ants') do |f| %>

  <div class="field">

    <%= label_tag :quantity %><br />

    <%= text_field_tag :quantity %>

  </div>

  <div class="actions">

    <%= submit_tag %>

  </div>

<% end %>

Mon problème ici est la gestion des erreurs, ne serais-ce qu'imposer à l'utilisateur de remplir le champs.

Comment faire?

[niahoo]

je connais pas rails mais bon, dans ton controlleur ben tu controles et si c'est pas bon tu renvoies la même vue en indiquant l'erreur, sinon tu passes à la validation.

Et tu rajoutes une validation en javascript aussi.

[Sephi-Chan]

Hm. Je pense que tu n'utilises pas les mécanismes du frameworks.
Peux-tu montrer l'action du contrôleur qui traite ce formulaire ?

De manière général, les mécanismes de ActiveRecord ont été déplacés dans ActiveModel, ce qui permet d'ajouter les différentes comportements des modèles à n'importe quel objet. Mais avant d'opter pour cette solution, il faudrait voir si c'est réellement nécessaire.


Ps : joyeux anniversaire !

[Maz]

Le formulaire renvois sur ça:

  def create

  	if current_user.city.ants.size

  		@decalage = 0

  	else

			@lastAnt = current_user.city.ants.find_all().last

			@decalage = @lastAnt.born - Time.now - 28800

			if @decalage < 0

				@decalage = 0

			end

		end

		@quantity = params[:quantity].to_i - 1

  	for i in 0 .. @quantity

  		current_user.city.ants.build(ort => "bip",

  					     :born => Time.now + i*6 + @decalage,

  					     :name => ActiveRecord::Base.connection.select_value("SELECT nextval('fourmis_" + current_user.username + "_id_seq')"))

  	end



    respond_to do |format|

      if current_user.city.save

        format.html { redirect_to user_ants_path, notice: 'Ant was successfully created.' }

        format.json { render json: @ant, status: :created, location: @ant }

      else

        format.html { render action: "new" }

        format.json { render json: @ant.errors, status: :unprocessable_entity }

      end

    end

  end

Je me rends comptes que j'ai encore laisser la "gestion d'erreur" généré par défaut (le if save else...). Je peut donc vérifier l'existence de la variable @ant.errors et affiché en conséquence. Mais comment définir que le champs "quantity" soit obligatoire?

Si en parlant de réellement nécessaire tu parles de ce fameux formulaire qui n'est pas lié a un active record, oui il l'est. En fait, c'est un jeu (étonnant hein?), On choisi combien de fourmis on veut créé, juste la quantité, le reste est géré par rails, la caste est choisi aléatoirement parmis 3 possibles, la vie est par défaut à 100, le nom est unique pour chaque joueur et va de 1 à beaucoup(c'est pourquoi j'utilise une sequence par joueur), et le calcul de la naissance(born) et un peu plus complexe à expliqué.

Je veut vraiment que le joueur ne choisisse que la quantité.

Et merci Sephi ,)

[Sephi-Chan]

Le formulaire renvois sur ça:

  def create

  	if current_user.city.ants.size

  		@decalage = 0

  	else

			@lastAnt = current_user.city.ants.find_all().last

			@decalage = @lastAnt.born - Time.now - 28800

			if @decalage < 0

				@decalage = 0

			end

		end

		@quantity = params[:quantity].to_i - 1

  	for i in 0 .. @quantity

  		current_user.city.ants.build(ort => "bip",

  					     :born => Time.now + i*6 + @decalage,

  					     :name => ActiveRecord::Base.connection.select_value("SELECT nextval('fourmis_" + current_user.username + "_id_seq')"))

  	end



    respond_to do |format|

      if current_user.city.save

        format.html { redirect_to user_ants_path, notice: 'Ant was successfully created.' }

        format.json { render json: @ant, status: :created, location: @ant }

      else

        format.html { render action: "new" }

        format.json { render json: @ant.errors, status: :unprocessable_entity }

      end

    end

  end

Je me rends comptes que j'ai encore laisser la "gestion d'erreur" généré par défaut (le if save else...). Je peut donc vérifier l'existence de la variable @ant.errors et affiché en conséquence. Mais comment définir que le champs "quantity" soit obligatoire?

Si en parlant de réellement nécessaire tu parles de ce fameux formulaire qui n'est pas lié a un active record, oui il l'est. En fait, c'est un jeu (étonnant hein?), On choisi combien de fourmis on veut créé, juste la quantité, le reste est géré par rails, la caste est choisi aléatoirement parmis 3 possibles, la vie est par défaut à 100, le nom est unique pour chaque joueur et va de 1 à beaucoup(c'est pourquoi j'utilise une sequence par joueur), et le calcul de la naissance(born) et un peu plus complexe à expliqué.

Je veut vraiment que le joueur ne choisisse que la quantité.

Ah et encore un problème auquel j'ai été confronté. En fait le nextval qui incrémente la sequence SQL n'est exécuté qu'une seule fois.... Je ne comprends pas. Si l'on demande 5fourmis, elles auront toutes le nom "10" par exemple, et si on en redemande 5 elles auront "11", etc...

Et merci Sephi ,)

Diantre, ce code est très perfectible ! Quelques pistes :

• Tu devrais commencer par choisir une langue pour nommer des variables. Bien sûr, c'est l'anglais qu'il faut choisir pour tirer au mieux profit du principe de convention over configuration.
  
• Ensuite, tu devrais plutôt utiliser les underscore comme séparateur (@last_ant plutôt que @lastAnt). C'est la norme en Ruby. La cohérence est l'alliée de la lisibilité.
  

Concernant le code en lui même, tu as une faille de logique majeure dans ton premier test. En effet, le else ne sera jamais exécuté puisque Ruby évalue toute expression comme true à l'exception de nil et false. Ainsi, tout nombre — que ce soit -1, 0 ou 1 — vaut true dans un test logique, idem pour une chaîne vide, un tableau vide, etc.

Pour ce genre de tests, utilise plutôt l'une des notations suivantes, ça rend le code plus lisible :

city.ants.empty? # Retourne true si size == 0

city.ants.any?   # Retourne true si size > 0

city.ants.many?  # Retourne true si size > 1

Ensuite, ta requête pour récupérer la dernière fourmi n'est pas efficace, tu peux la simplifier.

@lastAnt = current_user.city.ants.find_all().last # Pas bien !

@last_ant = current_user.city.ants.last           # Bien !

Ensuite, n'utilise pas les dates comme ça. Sers-toi de Ruby et de Rails ! Ici, ton 28800 n'évoque rien. Utilise plutôt 8.hours

# Pas bien !

@decalage = @lastAnt.born - Time.now - 28800

if @decalage < 0

  @decalage = 0

end



# Bien !

@time_shift = @last_ant.born - 8.hours.from_now

@time_shift = 0 if @time_shift < 0

Bon, maintenant que tout ça est dit : je pense que ton code est mauvais. Voici mes arguments :

• Cette action ne devrait pas rencontrer d'erreur (si je me trompe — car c'est possible :p — explique moi les erreurs potentielles). À ce titre, je n'utiliserais pas les validateurs mais plutôt la politique du Fail-fast, qui consiste à lancer des exceptions dans les cas anormaux. N'utilise la validation que quand un utilisateur de bonne foi peut être amené à corriger les informations saisies.
  
• Tu ne distribues pas assez les responsabilités. Ton contrôleur prend en charge des choses dont il ne devrait pas avoir conscience. Ton code est difficile à tester et on ne comprend pas ce qu'il fait d'un coup d'œil : définis des méthodes dans tes modèles.
  
• Enfin, cette action a-t-elle vraiment besoin de gérer les formats JSON et HTML ?
  

Voici la version que je te propose. N'hésite pas à me dire si elle ne colle pas à tes besoins :

def create

  @quantity = params[:quantity].to_i - 1

  current_user.city.product_ants!(@quantity)

  

  format.html { redirect_to(user_ants_path, notice: 'Ant was successfully created.') }

  format.json { render(json: true)

end





class Ant < ActiveRecord::Base

  PRODUCTION_DURATION = 6.seconds

  

  # Quelque chose de plus cross-database ne serait-il pas plus intéressant ?

  def self.next_name_for(user)

    sequence = "fourmis_#{user.username}_id_seq"

    query    = "SELECT nextval('#{sequence}')"

    ActiveRecord::Base.connection.select_value(query)

  end

end





class City < ActiveRecord::Base

  belongs_to :user

  has_many   :ants

  

  # Add few ants to the production queue.

  # Raise if something goes wrong.

  def product_ants!(count)

    # ...

  end

end

Je te laisse implémenter product_ants! car je n'ai pas bien compris tes histoires de 6 secondes et de 8 heures. Bien sûr, si tu optes — même partiellement — pour ma version du code, j'aimerais que tu me montres l'implémentation de cette méthode.

[Maz]

Merci BEAUCOUP, pour tous ces conseils, vraiment. J'ai pris bonnes notes et modifier, d'ailleurs pour montrer ma studiosité(pas stupidité!), j'avais déjà créé mes propres fonctions pour la creation de sequence et le nextval:

class City < ActiveRecord::Base

	def create_sequence_fourmis_id_seq

		ActiveRecord::Base.connection.execute("CREATE SEQUENCE fourmis_" + self.user.username + "_id_seq")

	end



	def nextval_fourmis_id_seq

		ActiveRecord::Base.connection.select_value("SELECT nextval('fourmis_" + self.user.username + "_id_seq')")

	end

end

Bien sur elle ne sont pas aussi bien que les tiennes, d'ailleurs je vais les utiliser (je n'avais pas penser à mentioner user en paramètre ,) ).

Pour ce qui est du JSON et l'autre, j'ai laissé par défaut ce que le scaffold m'as généré.

La seule erreur qui peut se produire est que le champs quantity ne soit pas remplis.

Je vais faire la méthode products! j'éditerais ce message au cas ou personne ne répondrais derrière.

[Sephi-Chan]

Merci BEAUCOUP, pour tous ces conseils, vraiment. J'ai pris bonnes notes et modifier, d'ailleurs pour montrer ma studiosité(pas stupidité!), j'avais déjà créé mes propres fonctions pour la creation de sequence et le nextval:

class City < ActiveRecord::Base

	def create_sequence_fourmis_id_seq

		ActiveRecord::Base.connection.execute("CREATE SEQUENCE fourmis_" + self.user.username + "_id_seq")

	end



	def nextval_fourmis_id_seq

		ActiveRecord::Base.connection.select_value("SELECT nextval('fourmis_" + self.user.username + "_id_seq')")

	end

end

Bien sur elle ne sont pas aussi bien que les tiennes, d'ailleurs je vais les utiliser (je n'avais pas penser à mentioner user en paramètre ,) ).

Pour ce qui est du JSON et l'autre, j'ai laissé par défaut ce que le scaffold m'as généré.

La seule erreur qui peut se produire est que le champs quantity ne soit pas remplis.

Je vais faire la méthode products! j'éditerais ce message au cas ou personne ne répondrais derrière.

Donc le cas d'erreur n'est pas celui d'un utilisateur de bonne foi puisque le champ aurait forcément une valeur par défaut (que ce soit dans un champ caché ou dans le formulaire de traitement, peu importe). D'où l'intérêt de lancer une exception si ça foire. Le code ne se soucie que des cas d'utilisation réalistes, ce qui le rend plus simple et plus efficace.

Concernant tes fonctions, je ne les trouve pas très bien nommées et je trouve plus cohérent que ce soit la classe Ant qui soit capable de déterminer le nom d'une fourmi.

Méfie-toi des scaffold. C'est cool pour apprendre mais les CRUD ne sont pas tes meilleurs amis quand tu codes un jeu. Pose toi toujours la question de quel format est utile à rendre. Et quand tu veux rendre du JSON un peu complexe, utilise la gem Rabl.

[Maz]

Une question: quel est la différence entre @variable et variable? Une question de portée?

[Sephi-Chan]

Shame on you ! @foo est une variable d'instance ($this->foo) alors que sans, c'est local.
$foo est globale et @@foo est une variable de classe (statique).

[Maz]

Shame on me...
J'ai beaucoup de mal avec "self". Voici le code actuel sur lequel je bloques:

# Add few ants to the production queue.

def product_ants!(quantity)

	@time_shift = 0

	unless self.ants.empty?

		@time_shift = self.ants.last.born - 8.hours.from_now if self.ants.last.born - 8.hours.from_now < 0

	end

	@quantity = quantity.to_i - 1

	uncached do

		0.upto(@quantity) do |i|

			self.ants.build(ort => Ant::SeORTS[rand(3)],

				        :born => Time.now + i*Ant:RODUCTION_DURATION + @time_shift,

				        :name => Ant.next_name_for(self.user))

		end

	end

end

unless self.save

	raise ArgumentError, "Error occured"

end

le self.save me renvoi une erreur "undefined method"

Ce que je ne comprends pas, c'est que le self.ants.empty? ne renvois pas d'erreur, mais si je fait un unless self.ants.empty? juste avant le raise (c'était juste pour essayer), ça me renvois une erreur...

undefined method `ants' for #<Class:0xa7e9fc8>

J'ai d'abord penser que la désactivation de la mise en cache effacer en même temps le cache actuel donc perte de l'objet city en cours ou un truc du genre, mais même en désactivant la désactivation(wow), ça ne fonctionnes pas.

Shame on me? =(

EDIT: Shame on me!! Bordel je me hais sur ce coup là!! Je la laisse pour me faire maltraîter!

EDIT²: uncached do ne fonctionnes pas non plus dans le modèle:

undefined method `uncached' for #<City:0xa48457c>

EDIT³: voici ma fonction finale, si tu as encore des conseils à me donner:

# Add few ants to the production queue.

def product_ants!(quantity)

	@time_shift = 0

	unless self.ants.empty?

		@time_shift = self.ants.last.born - 8.hours.from_now + Ant:RODUCTION_DURATION if self.ants.last.born > 8.hours.from_now

	end

	@quantity = quantity.to_i - 1

	ActiveRecord::Base.connection.disable_query_cache!

	0.upto(@quantity) do |i|

		self.ants.build(ort => Ant::SORTS[rand(3)],

			        :born => 8.hours.from_now + i*Ant:RODUCTION_DURATION + @time_shift,

			        :name => Ant.next_name_for(self.user))

	end

	ActiveRecord::Base.connection.enable_query_cache!



	unless self.save

		raise ArgumentError, "Error occured"

	end

end

L'histoire du calcul du temps si tu veut le savoir c'est:
Une fourmis met 8h à naître, donc si tu demandes à en créé 20 d'un coup, pour que ce soit un peu plus "rôle-play", j'ai limité la ponte à 1fourmis/6secondes.
Le problème avec un système pareil c'est que si tu cré 5fourmis et que de suite après l'envoi du formulaire tu redemandes 5fourmis, les temps vont se chevaucher, donc je dois vérifier s'il y a encore des fourmis en cours de ponte (si la naissance est prévu dans plus de 8h), et si c'est le cas, ajouter les fourmis du second envoi à la suite du premier.

Là ça fonctionnes impeccablement, juste cette histoire de uncached à laquelle je n'ai pas encore trouvé la réponse...

EDIT⁴: ActiveRecord::Base.uncached fonctionnes... bizarrement...