-
Notifications
You must be signed in to change notification settings - Fork 22
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
Permettre de s'abonner plus d'une heure, un jour ou un mois avec Stri… #126
base: master
Are you sure you want to change the base?
Conversation
…pe... Depuis le temps !
@@ -291,6 +291,8 @@ function presta_stripe_call_request_dist($id_transaction, $transaction_hash, $co | |||
} | |||
} | |||
|
|||
$interval_count = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mais du coup là tu l'initialises à 1 mais tu ne l'utilises jamais ensuite non ? ça devrait être le fallback de si c'est pas fourni dans le tableau
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce sont les plugins qui font l'interface entre les commandes et les abonnements qui sont censés les manipuler, non ?
@@ -321,7 +323,7 @@ function presta_stripe_call_request_dist($id_transaction, $transaction_hash, $co | |||
'unit_amount' => $montant_echeance, | |||
'recurring' => [ | |||
'interval' => $interval, | |||
'interval_count' => 1, | |||
'interval_count' => $echeance['interval_count'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quelque chose comme $echeance['interval_count'] ?? $interval_count
?
autre question : est-ce que dans le tableau qui est générique et qui vient de "decrire_echeance", on doit attendre une clé qui s'appelle "interval_count" et qui est propre à stripe, ou bien on peut trouver un nom qui est plus générique et qui sémantiquement pourrait s'appliquer à tout ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alors, tu peux soulever le chantier de la cohérence des noms des variables pour "decrire_echeance", parce qu'entre les variables en français ("montant", "montant_init"), les variables en anglais ("count",, "count_init","date_start"), et les variables dans les 2 langues abregées, ("frequ", "nb"), j'en n'ai pas vraiment trouvé, perso, de cohérence. J'ai l'impression qu'elles ont été ajoutées en fonction de ce qu'amenaient les API de paiement. Alors j'ai fait pareil avec Stripe.
Ils font comment les autres prestataires pour proposer des abonnements trimestriels par exemple ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quelque chose comme
$echeance['interval_count'] ?? $interval_count
?
En fait, tu as raison. Je corrige ça...
La raison d'être du plugin c'est de proposer une API commune indépendante du prestataire bancaire qu'on va utiliser ensuite. C'est à dire qu'on a pas pour but de supporter toutes les fonctionnalités proposées par un prestataire donné, mais uniquement les fonctionnalités qu'on pourra supporter avec l'ensemble des prestataire, car cela veut dire qu'on peut ensuite switcher facilement sans avoir à toucher à son code. Stripe permet plein de choses que les autres prestataires ne permettent pas, donc on utilise qu'un petite partie des fonctionnalités. En particulier les autres prestataires ne permettent pas d'avoir un abonnement récurrent tous les N mois, c'est donc la raison pour laquelle le N est forcé à 1 arbitrairement dans le code. Si on supporte N en option dans l'API stripe alors quelqu'un qui l'utilise et change plus tard de prestataire va avoir un soucis. Je vais prendre le temps de peser le pour et le contre, mais du coup je ne pense pas que cette PR ait sa place dans le plugin bank car ça casserait la portabilité. Peut-être il faut envisager un plugin bank_stripe qui permettrait le support de plus de fonctionnalités que le socle commun et éviterait de faire croire que "puisque mon code marche avec stripe et le plugin bank, je peux changer de prestataire sans problème" ? |
OK, Merci pour ta réponse. Peut-être pourrait-on mettre dans le "pour" le fait qu'à partir du moment où un prestataire propose une réccurence, la réccurence annuelle doit faire partie des options basiques qui doivent être proposée. Or pour cela, il faut attribuer "12" à la variable interval_count |
Nan quand c'est 12 mois, c'est toi en interne avant qui est censé le détecter, et dans la description de l'échéancier t'es censé mettre "yearly" et non par mois : https://github.com/nursit/bank/blob/master/abos/decrire_echeance.php#L27 |
OK, J'ai fait la confusion parce que le plugin "abonnement" ne permet que de proposer des offres horaires, journalières ou mensuelles. Mais sérieux, ça ne te semble pas élémentaire ? Mon cas de figure est de proposer un abonnement trimestriel. Et puis la modif est ridiculement courte et simple. J'ai peur de trop mal connaître Paypal et Payzen, mais je ne vois pas comment ces prestataires ne peuvent pas proposer une option aussi élémentaire. |
…arte de paiement liée à un abonnement sous Stripe
…arte de paiement liée à un abonnement sous Stripe / 2
Pour permettre de s'abonner plus d'une heure, un jour ou un mois avec Stripe, il faut prendre en compte la variable "interval_count" trop autoritairement passé à "1" sur le plugin actuel.