Partial refunds #2230
Partial refunds #2230
Conversation
| @@ -42,7 +42,7 @@ def get_initial_payment_url(self) -> str: | |||
|
|
|||
| return result["link"] | |||
|
|
|||
| def refund(self) -> None: | |||
| def refund(self, amount: Decimal | None = None) -> None: | |||
There was a problem hiding this comment.
А зачем здесь меняем сигнатуру?
There was a problem hiding this comment.
Потому что у родительского Bank сигнатура изменилась и линтер ругается, что сигнатуры не совпадают , подумала, что так правильнее, чем повесить type: ignore
|
Уведомления о частичных и полных возвратах отправляются в одном месте , предлагаю к шаблону кроме изначальной цены добавить сумму текущего возврата и сколько средств возможно нужно вернуть |
|
Сейчас есть всё для частичных возвратов, но полные возвраты в админке по-прежнему работают. ПР уже довольно большой. Если у тебя нет никаких дополнений, я думаю, что мы могли бы слить его уже сейчас. А добавление частичных возвратов в админку я бы сделала в отдельном ПР, как на это смотришь? Следующие шаги вижу так:
Раньше я делала такое так: при нажатии на кнопку происходит переход на страницу с формой, где вводятся нужные данные и редирект на страницу заказа после. Я понимаю, что кастомайзить админку - боль, но у меня такой опыт есть и если есть вариант лучше - подскажи, что посмотреть, пожалуйста |
Я бы весь код кастомизации спрятал бы в отдельную модельку для админки. Сделал бы прокси-модель к рефандам, типа Таким образом мы получим вывод всех возвратов по заказу в привычном интерфейсе админки и ноль кастомизаций. |
|
@f213 комментарии поправлю чуть позже, а пока вопрос: в сервисе возврата происходит много операций, кажется, что стоит обернуть это всё в transaction.atomic, чтобы при ошибке на любом этапе данные в базе не расходились с реальным положением дел или это не сделано по какой-то причине? |
Не вижу ни одной причины так не сделать. |
| bank_id=self.order.bank_id, | ||
| ) | ||
|
|
||
| def mark_order_as_not_paid_if_needed(self) -> None: |
There was a problem hiding this comment.
Я думаю от всей этой логики можно избавиться, как и от поля unpaid. Мы пытались на его основе построить дешборды для возвратов, то так и не сделали. В новой версии приложения записи Refund полностью его заменят.
Давай удалим метод и поле unpaid.
There was a problem hiding this comment.
Тогда paid мы не сбрасываем, когда заказ полностью вернули? Сейчас дата становится None и к этому привязаны различные штуки вроде обновление тегов для даши
Ещё такой вопрос, если мы делаем возврат для ещё не оплаченного заказа (paid=None), то к возврату доступно 0 и в записи о возврате - 0?
There was a problem hiding this comment.
UPD: дропнула unpaid, но метод пока оставила, чтобы чистить paid при полном возврате: не уверена, что ничего не сломается, если это убрать (как минимум ломаются тесты с amo)
А вопрос по возврату неоплаченных заказов актуальный, не знаю, может ли быть такой кейс в реальной жизни. Сейчас для таких заказов можно добавлять возвраты до полной стоимости заказа и это кажется мне неверным
There was a problem hiding this comment.
Так, я тут нашёл ещё одну важную проблему. У нас сейчас написана куча дешбордов, в которых мы считаем деньги. К примеру, чтобы посчитать деньги за период, мы берём все заказы, у которых paid is not null and between period_start and period_end и делаем SUM(price * acquiring_percent).
Получается, что как только мы перейдём на новую систему возвратов, все дешборды начнут врать, т.к. при создании частичного возврата price меняться не будет.
Я предлагаю добавить денормализацию:
- При создании
refundуменьшать order.price на его сумму paidпри полном рефанде не сбрасывать, чтобы было видно, что заказ когда-то был оплачен.order.priceпри этом делать нулевым.
Что думаешь?
There was a problem hiding this comment.
Супер, это позволит упростить код сервиса ещё немного и полностью отказаться от available_to_refund_amount: по price уже будет понятно, сколько можно вернуть
И ещё раз четче сформулирую вопрос: если заказ не был оплачен (paid=None), а мы пытаемся сделать возврат - что происходит? В моем понимании, нам ещё не поступили деньги, а значит мы не можем их вернуть, но должны закрыть доступ к продукту.
Для таких заказов возврат делается на сумму 0 и всё равно записывается в логах?
There was a problem hiding this comment.
И ещё раз четче сформулирую вопрос: если заказ не был оплачен (paid=None), а мы пытаемся сделать возврат - что происходит? В моем понимании, нам ещё не поступили деньги, а значит мы не можем их вернуть, но должны закрыть доступ к продукту.
Да, всё так.
На самом деле это довольно редкий кейс — заказ может быть paid=None и shipped is not None только в случае больших b2b-заказов, когда мы допускаем учеников до занятий до поступления оплаты. Не помню ни одного раза, чтобы при этом нужно было оформлять возврат. Но да, лучше сделать.
На всякий случай не забывай, чт paid может быть not None, а price=0 — это ученики, бесплатно допущенные до курсов. Туда попадают ранние чтецы или, к примеру, все кого мы отправляем учиться из fands. Вряд ли конечно там будут возвраты, но лучше бы тоже учесть.
|
Я учла все комментарии, а ещё решила админку сделать здесь(через инлайн там действительно добавилось не очень много нового кода, а троттлинг я перенесла прямо в сервис возвратов, и кажется, это наиболее логично) Смущает только один момент, в админке при мисклике (например, если дважды быстро нажать на сохранить), в инлайнах могут создаваться дубли записей возврата. Для частичного возврата, если сумма может быть возвращена дважды - так и произойдёт, но вероятность такого достаточно мала |
Давай может и её заблокируем? Можно в тротлинг к примеру добавить логику, что мы можем делать только один возврат в течение 10 секунд. |
| class OrderRefundActionThrottle(ConfigurableThrottlingMixin, UserRateThrottle): # type: ignore | ||
| """Throttle for order's admin action `refund`.""" | ||
|
|
||
| scope = "order-refund" |
There was a problem hiding this comment.
Кажется, скоуп стоит выпилить ещё и отсюда.
| bank_id=self.order.bank_id, | ||
| ) | ||
|
|
||
| def mark_order_as_not_paid_if_needed(self) -> None: |
There was a problem hiding this comment.
Так, я тут нашёл ещё одну важную проблему. У нас сейчас написана куча дешбордов, в которых мы считаем деньги. К примеру, чтобы посчитать деньги за период, мы берём все заказы, у которых paid is not null and between period_start and period_end и делаем SUM(price * acquiring_percent).
Получается, что как только мы перейдём на новую систему возвратов, все дешборды начнут врать, т.к. при создании частичного возврата price меняться не будет.
Я предлагаю добавить денормализацию:
- При создании
refundуменьшать order.price на его сумму paidпри полном рефанде не сбрасывать, чтобы было видно, что заказ когда-то был оплачен.order.priceпри этом делать нулевым.
Что думаешь?
src/apps/orders/models/order.py
Outdated
| return self.filter(paid__isnull=False, price__gt=0) | ||
|
|
||
| def not_paid(self) -> "OrderQuerySet": | ||
| return self.filter(Q(paid__isnull=False, price=0) | Q(paid__isnull=True)) |
There was a problem hiding this comment.
Это слабо сообветствует бизнесу. Заказ вполне может быть оплачен за ноль денег — это внутренине заказы для тестирования, или бесплатно допущенные студенты, к примеру из fands.
Подскажи, какой смысл этого изменения?
There was a problem hiding this comment.
Ты прав, меня тогда спутали тестовые данные и мне казалось, что даже у заказов для чтецов и нас устанавливается ненулевая цена
Но ещё меня смутил вот этот тест, потому что сейчас новым заказам в амо привязывается лид из возвращенных заказов (или не закрытых)
Сейчас если заказы с paid is not None для этой же сделки существуют - новый заказ не отправится в амо, раньше в это условие не попадали заказы с возвратами, потому что мы сбрасывали дату платежа, теперь нужно другое условие
Кажется, это важно и чтобы всё было правильно с точки зрения бизнеса, можно учитывать здесь только заказы с ненулевой ценой, вот так:
if Order.objects.paid().same_deal(order=self.order).filter(amocrm_lead__isnull=False, price__gt=0).exists(): # we have other paid orders for the same deal
Что думаешь?
|
@f213 по коду всё, можем мерджить. Из важного: в админке в списке по-прежнему можно сделать полный возрат, я поправила название действия на "Сделать полный возврат", чтобы не возникло путаницы Осталось поправить шаблон в постмарке и всё готово: и заменить
на
так будет окей? |
Супер!
Да, вполне. Думаю, сегодня я уже не успею, а завтра будем тестить |
OrderRefunder теперь обрабатывает и полный, и частичный возвраты(если указан amount), так, например, можно будет оставить возможность полного возврата в списке
В админку пока не выводила, хочу окнуть изменения сервиса: сейчас я просто храню сумму прошлых возвратов в поле refund_amount заказа, а историю возвратов записываю в LogEntry.
Вижу два варианта:
Кажется, что первый вариант реализовать проще, что думаешь?