[MSJ-852] divestment history in convict #735
Conversation
Evolution poste réu des ambassadeurs: pour un probationnaire donné, s’il est lié à un ou des TJ, ce sont seulement eux qui peuvent valider le dessaisissement. S’il n’est pas lié à des TJ c’est son ou ses SPIPs qui doivent le faire. Même si le le SPIP n’a pas son mot a dire, il est quand même prévenu. —> Proposition : Le SPIP est passé en automatiquement validé s’il n’a pas de réponse à donner.
|
|
||
| @organization_divestment.accept | ||
| handle_divestment_state | ||
| end | ||
|
|
||
| def refuse | ||
| def refuse(comment = nil) |
There was a problem hiding this comment.
Ca vaudrait sans doute le coup au minimum de séparer en plusieurs services : OrganizationDivestmentAcceptationService et même chose pour le refus.
There was a problem hiding this comment.
Et peut être qu'ensuite on pourrait les clarifier un peu avec des sous méthodes systématiques pour chaque étape. Ex:
return_unless_pending
update_if_comment
refuse_divestment
etc.
| # resource_class.with_less_stuff | ||
| # end | ||
| # end | ||
| def scoped_resource |
There was a problem hiding this comment.
Il y a besoin de ça pourquoi par curiosité ? Pour la state_machine ? C'est pas géré nativement ?
There was a problem hiding this comment.
Et aussi, il n'y a pas tous ces statuts sur les divestments
There was a problem hiding this comment.
Pour voir les states accesibles, state_machine a vehicle.state_paths.to_states aussi
There was a problem hiding this comment.
c'est pour gerer l'ordre d'affichage des resources
There was a problem hiding this comment.
j'ai rien trouvé d'autre dans administrate
There was a problem hiding this comment.
Et pour les autres commentaires sur le filtrage par state dispo et le fait qu'il y ait moins de states que ça dans divestment?
There was a problem hiding this comment.
si tu envisage de ne pas hardcodé les values, je peux essayer mais c'est pas trivial
There was a problem hiding this comment.
Sisi, je t'ai mis la méthode mise à dispo par la gem state machines :) vehicle.state_paths.to_states
| @@ -27,7 +37,7 @@ def divestment_refused | |||
| @organization_divestment = params[:organization_divestment] | |||
| @comment = @organization_divestment.comment | |||
| @current_user = params[:current_user] | |||
| @convict = divestment.convict | |||
| @convict = @divestment.convict | |||
There was a problem hiding this comment.
je sais pas je check dans le html correspondant
There was a problem hiding this comment.
Bin même si elles sont dans le html tu peux faire @divestment.convict dans le html aussi :) C'est plutôt une bonne pratique de ne pas multiplier les variables d'instance :)
| @@ -12,25 +12,31 @@ class OrganizationDivestment < ApplicationRecord | |||
| .where(organization_divestments: { state: 'pending' }, divestments: { state: 'pending' }) | |||
| } | |||
|
|
|||
| scope :reminders_due, lambda { | |||
| where(state: 'pending') | |||
| .where('last_reminder_email_at IS NULL OR last_reminder_email_at <= ?', 5.days.ago) | |||
There was a problem hiding this comment.
Pour le 5.days.ago peut être mettre le delai dans une constance ?!
| return false unless @organization_divestment.pending? && @divestment.pending? | ||
| def accept(comment = nil) | ||
| ActiveRecord::Base.transaction do | ||
| return false unless @organization_divestment.unanswered? && @divestment.pending? |
There was a problem hiding this comment.
Pas possible de gérer ça via des validations?
| # rubocop:disable Metrics/CyclomaticComplexity | ||
| def refuse(comment = nil) | ||
| ActiveRecord::Base.transaction do | ||
| return false unless @organization_divestment.unanswered? && @divestment.pending? |
There was a problem hiding this comment.
Cf commentaires de l'accept
| @@ -46,17 +56,17 @@ def handle_undecided_divestment | |||
| return unless @divestment.refused? | |||
|
|
|||
| @divestment.organization_divestments.where(state: :pending).each do |organization_divestment| | |||
| organization_divestment.ignore | |||
| organization_divestment.update(comment: 'orga divestment ignored because divestment was refused') | |||
| organization_divestment.ignore! # Assuming ignore! is a method you implement that can raise exceptions on failure | |||
There was a problem hiding this comment.
Pourquoi ce commentaire ?
| @@ -25,6 +25,12 @@ | |||
| subject(:service) { DivestmentStateService.new(tj_organization_divestment, admin) } | |||
|
|
|||
| describe 'accept divestment' do | |||
| it 'handle comment' do | |||
There was a problem hiding this comment.
On est un peu light en tests non ?! 😇
There was a problem hiding this comment.
tu rajouterai quoi comme test parce que j'ai l'impression d'avoir fait le tour des cas interessants
There was a problem hiding this comment.
Ce serait cool qu'on ait des tests de features sur tout le processus, pas simplement sur la creation d'un dessaisissement par exemple :)
Et sinon je dirai qu'une bonne première approche serait de tester ce que j'ai repéré comme petits bugs lors de ma review produit :) On peut déjà commencer comme ça si ça te dit.
Arahir
force-pushed
the
feature/divestment
branch
from
bfb3198
to
fe3fb00
Compare
No description provided.