wip: paid subscriptions system and articles #467

DioFun · 2024-07-04T17:28:26Z

Le but est d'avoir un suivi et des correctifs aux fur et à mesure pour le moment quant au système d'abonnement payant.
Développement de la gestion des articles et abonnements payants.
La gestion des remboursements sera effectuée dans une autre PR

links to #455

Co-authored-by: DioFun <[email protected]> Co-authored-by: Nymous <[email protected]>

todo: fix tests

…le table

codecov · 2024-07-05T09:26:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (01e1374) to head (1a799a5).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##            master      #467    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           26        48    +22     
  Lines          337       663   +326     
  Branches        35        66    +31     
==========================================
+ Hits           337       663   +326

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…commit

Letiste

First batch of comments, I didn't finish the review yet. I didn't review the views because the focus here is more on the sales logic than adding some forms. I still need to review the sale/refund logic + the invoice generation

Letiste · 2024-07-14T12:45:53Z

db/migrate/20240720124420_create_sale_article_details.rb

+    # create_join_table :sales, :articles, column_options: { foreign_key: true } do |t|
+    #   t.integer :quantity, null: false
+    # end


Why is this commented out?

At first, we tried to use the create_join_table method but that do not create a primary, auto-increment key thus when we tried to delete a record from the join table it was a fail. So I decided to switch to a classic table creation with create_table in order to have such a key.

Letiste · 2024-07-14T12:53:57Z

db/migrate/20240720124610_create_active_storage_tables.active_storage.rb

The current config for active storage in production is to store the uploaded files on disk (see https://github.com/rezoleo/lea5/blob/master/config/environments/production.rb#L41-L42). Would that give us enough persistence for what we need to store?

Yes we want to store less than 100 MB of pdf (pdf are very light)

What happens if we lose the disk? Do we have any backup mechanism in place to restore the pdfs (or regenerate them)?

bad things I guess. The pdf can be regenerated using the json stored in the invoice object, but in case we also lose those, i think it would be a good idea to also send the invoices in a folder on another server when we create them

It would surely be host on the PVE which is backed up but that are consideration when we will deploy it.

Letiste · 2024-07-14T12:56:53Z

app/models/article.rb

+
+  validates :name, presence: true, allow_blank: false
+  validates :price, presence: true, allow_blank: false,
+                    numericality: { greater_than_or_equal_to: 0, only_integer: true, message: 'Must be a positive


Should the price be strictly greater than 0?

It's something that should be discuss. In a near or far future if we want to manage stocks of products maybe we'd like to sell things for free.

If we are not sure it's something we need now, I would suggest allowing a price only strictly greater than 0. I think it would simplify some logic (not handling division by zero and a total price of zero). And that won't block us from adapting the logic in the future to support a price of 0

Letiste · 2024-07-14T13:01:11Z

app/models/article.rb

+  validates :name, presence: true, allow_blank: false
+  validates :price, presence: true, allow_blank: false,
+                    numericality: { greater_than_or_equal_to: 0, only_integer: true, message: 'Must be a positive
+                     number. Maximum 2 numbers after comma' }


If we only allow integers with only_integer: true, what does 2 numbers after comma mean here?

In the UI, the user will be ask to enter a decimal number and then it'll be multiply by 100

Ah the price is in cents, I didn't catch that, makes sense! So we have two different validations:

in the backend, we only expect integers

in the frontend, we only expect float with a maximum of 2 decimals

In this case, I would change the error message in the backend to something like Must be a positive integer because it shouldn't be aware of how this integer is computed in the frontend. And have some logic in the frontend to display an error when the price is more than 2 decimals. What do you think?

Yes I agree but I'm not sure on how to deal with that to change the error in front.

Letiste · 2024-07-14T13:02:42Z

app/models/invoice.rb

+# frozen_string_literal: true
+
+class Invoice < ApplicationRecord
+  has_one :sale, dependent: :restrict_with_exception


Why do we use restrict_with_exception here and restrict_with_error in other places? Should we prefer one to the other?

The place we use restrict_with_error is because when we try to delete the record we don't know if it will fail or not and if it fail we soft delete it. But here, it's not planned to be deleted in the code so it's with an exception.

Got it! I think it's worth adding some comments on that, and some tests on the destroy code path (not only soft_delete). Would use restrict_with_exception everywhere and explicitly handle the exception and soft delete when it happens be more explicit? Not sure if it's very ruby-like though, I will defer to @nymous for this

Thinking about it again, if it's expected to try to delete an article but we should block the deletion when it's currently used by a sale, having an error instead of an exception makes more sense

Yes that has been raised by @nymous in one of his comment

app/models/user.rb

test/controllers/admin/articles_controller_test.rb

test/models/article_test.rb

test/models/refund_test.rb

Letiste · 2024-07-16T05:44:03Z

app/models/sale.rb

+  def update_total_price
+    total = 0
+    articles_sales.each do |rec|
+      total += rec.quantity * Article.find(rec.article_id).price
+    end
+    sales_subscription_offers.each do |rec|
+      total += rec.quantity * SubscriptionOffer.find(rec.subscription_offer.id).price
+    end
+    self.total_price = total
+  end


it's a bit of a code smell here because we need to call update_total_price before getting the total price. we could compute this value every time we get the total price instead, we would have the guarantee always to have the correct total price.
one benefit of being explicit on the update of the total price is that we don't compute it every time we get the total price but we don't need this kind of performance optimization (and we would need to adapt the logic anyway to guarantee that the total price is always up-to-date)

We call for update_total_price when there is no subscription offers nor articles to be add to the sale so there will not have any update of the price after. I agree that is not quite convincing but I did not find anything else to do so.

Do we need to have the total price in the database? The total price can be calculated at runtime by summing the price of articles and subscriptions, keeping it as a column in the database is redundant and we might have consistency issues (the total price of the sale is different from the sum of the articles' and subscriptions' price because we forgot to update the total price somewhere). And we can change update_total_price to be a getter (renaming it to total_price and returning the computed total price)

I agree for the getter, it sounds more idiomatic, but I believe that storing the total price is still a good idea : first, if articles are somehow lost, or an unexpected change happen (should not happen but who knows), we still have the accurate value of one of the most important thing : how much we were paied, without relying on some logic.

The fact is each time we would like to show a sale we need to calculate again the total price of the sale. That would be redundant because once a sale is created we want it to be immutable so after the creation the price couldn't be changed so there is no need to update the price later we just have to calculate it when we create it and it enables to avoid recalculate it each time we want to display which is the case when we show a user, we'd like to show every sales and their total price.

I don't think recalculating it every time is an issue. We certainly don't need that kind of optimization (we wouldn't even notice it). Making the code more resilient to bugs is more important in my opinion.

Regarding storage issues, we have a redundancy with the pdf if we need to know the price. I would say it's good enough to make sure we can't lose the invoices.

My comment is not blocking in any way, I don't mind keeping it like this

nymous

Same, just posting what I wrote for now (also you pushed after I started to review so some comments might be outdated ^^')

nymous · 2024-07-14T13:35:01Z

app/controllers/admin/articles_controller.rb

+    def destroy
+      @article = Article.find(params[:id])
+      authorize! :destroy, @article
+      @article.soft_delete unless @article.destroy


Maybe add a comment to explain why we do this.
Alternative way to write it, not really convinced by it but at least it reads from left to right (and there isn't a side-effect in the conditional):

Suggested change

@article.soft_delete unless @article.destroy

@article.destroy or @article.soft_delete

nymous · 2024-07-14T13:36:51Z

app/controllers/admin/dashboard_controller.rb

+module Admin
+  class DashboardController < ApplicationController
+    def index
+      authorize! :manage, :all


Is this a temporary permission and we will check more granularly, or is the admin dashboard only intended for superadmins?

That is temporary permission for now

nymous · 2024-07-14T13:38:20Z

app/controllers/admin/payment_methods_controller.rb

+    def destroy
+      @payment_method = PaymentMethod.find(params[:id])
+      authorize! :destroy, @payment_method
+      @payment_method.soft_delete unless @payment_method.destroy


Same thing here, maybe comment, maybe write it differently

Suggested change

@payment_method.soft_delete unless @payment_method.destroy

@payment_method.destroy or @payment_method.soft_delete

nymous · 2024-07-14T13:39:01Z

app/controllers/admin/subscription_offers_controller.rb

+    def destroy
+      @subscription_offer = SubscriptionOffer.find(params[:id])
+      authorize! :destroy, @subscription_offer
+      @subscription_offer.soft_delete unless @subscription_offer.destroy


Same, comment or

Suggested change

@subscription_offer.soft_delete unless @subscription_offer.destroy

@subscription_offer.destroy or @subscription_offer.soft_delete

app/views/sales/new.html.erb

nymous · 2024-07-14T14:16:31Z

app/controllers/sales_controller.rb

+    params.require(:sale).permit(:duration, :payment_method_id, articles_sales_attributes: [:article_id, :quantity])
+  end
+
+  def reformated_params


The params methods could probably do with some comments and vertical space to separate blocks ^^'

nymous · 2024-07-14T14:17:18Z

app/controllers/sales_controller.rb

+  # rubocop:disable Metrics/AbcSize
+  def create
+    @sale = @owner.sales_as_client.new(reformated_params)
+    @sale.update_total_price


Ooooh, it's dangerous to have to think to do this before saving, can this be moved to a before_save or something?

Has been moved but not sure if it's enough

nymous · 2024-07-14T14:19:57Z

app/models/article.rb

+  validates :name, presence: true, allow_blank: false
+  validates :price, presence: true, allow_blank: false,
+                    numericality: { greater_than_or_equal_to: 0, only_integer: true, message: 'Must be a positive
+                     number. Maximum 2 numbers after comma' }


Not sure the error message is up-to-date with the fact that we store it as cents.

In the UI, the user will be ask to enter a decimal number and then it'll be multiply by 100

app/models/sale.rb

app/controllers/sales_controller.rb

…d payment_method

…t deletion tests

…ash generation for sale

… is negative

Letiste · 2024-08-10T11:49:31Z

app/controllers/sales_controller.rb

+    unless @sale.generate(duration: params[:sale][:duration], seller: current_user)
+      return redirect_to :new_user_sale, user: @user, status: :unprocessable_entity
+    end
+    return redirect_to :new_user_sale, user: @user, status: :unprocessable_entity if @sale.empty?


what happens if we try to call generate on an empty sale? Before generating it, should we check if the sale isn't empty?

Letiste · 2024-08-10T11:50:13Z

app/javascript/controllers/sales_controller.js

+    // newArticle.getElementById("sale_article_id_new").id = `sale_article_id_${this.nextId}`
+    // newArticle.getElementById("sale_quantity_new").id = `sale_quantity_${this.nextId}`


can this be removed?

app/models/articles_sale.rb

Letiste · 2024-08-10T12:00:13Z

app/models/sale.rb

+  def compute_total_price
+    total = 0
+    articles_sales.each do |rec|
+      total += rec.quantity * Article.find(rec.article_id).price
+    end
+    sales_subscription_offers.each do |rec|
+      total += rec.quantity * SubscriptionOffer.find(rec.subscription_offer.id).price
+    end
+    total
+  end


We don't care about that at our scale because we are talking about one or two articles, but I think here we would make a request to the database per article/subscription. That can be a problem once you get to dozens of articles.
We don't need to change anything for our case though, it's simpler like that

Letiste · 2024-08-10T12:05:07Z

app/models/sale.rb

+  def generate_sales_subscription_offers(duration)
+    subscription_offers = SubscriptionOffer.order(duration: :desc)
+    if subscription_offers.empty?
+      errors.add(:base, 'There are no subscription offers registered!')
+      return false
+    end
+    subscription_offers.each do |offer|
+      break if duration.zero?
+
+      quantity = duration / offer.duration
+      if quantity.positive?
+        sales_subscription_offers.new(subscription_offer_id: offer.id, quantity: quantity)
+        duration -= quantity * offer.duration
+      end
+    end
+    return true if duration.zero?
+
+    errors.add(:base, 'Subscription offers are not exhaustive!')
+    false
+  end


I'm a bit lost trying to understand what this function is doing. What does the duration parameter represent? Why do we leave the loop once the duration is zero? Why do we return false when there is an error, should we instead throw if it's unexpected?

This function aims to generate the subiscription_offer for a given duration, duration is the number of months the subscription last. Example : an adherent want to pay for 13 months and we have two different offer 12 months is 50€ and 1 month is 5€ then it will create a sales_subscription_offers of 12 months and of 1 months. So we leave the loop when reach 0 because we do not need to continue as we already split the sale into the different offers we have. Lastly, we return false, because it's quite obscure the error process and the throw in my case i'd like just to send a message to the user not crash the website.

app/models/setting.rb

Letiste · 2024-08-10T12:08:01Z

app/models/subscription.rb

+  # validate :cannot_change_after_cancelled, on: :update

-  def cancel!
-    self.cancelled_at = Time.current
-    save!
+  def user
+    sale.client
  end

-  private
+  # def cancel!
+  #   self.cancelled_at = Time.current
+  #   save!
+  # end

-  def cannot_change_after_cancelled
-    return if cancelled_at_was.nil?
+  # private

-    errors.add(:cancelled_at, 'Subscription has already been cancelled')
-  end
+  # def cannot_change_after_cancelled
+  #   return if cancelled_at_was.nil?
+  #
+  #   errors.add(:cancelled_at, 'Subscription has already been cancelled')
+  # end


Can we delete the comments if it's not used?

I just commented it out for now because we would surely use it again when we'll make the refund module.

DioFun and others added 7 commits June 20, 2024 19:00

wip: begin of migration creation

01e20d1

feat: creating migration (work in progress)

5a8e0df

Co-authored-by: DioFun <[email protected]> Co-authored-by: Nymous <[email protected]>

fix: relations that wasn't well defined, fixtures fix

f76fb69

todo: fix tests

Remove user_id from subscription and get subscriptions through the sa…

fda8cd9

…le table

fix: through association was define after a call to it

9f884e5

Add subscriptions modules routes

472b9a8

fix rubocop problems

c82f46e

nymous marked this pull request as draft July 4, 2024 18:11

DioFun added 5 commits July 4, 2024 21:29

feat: creation and deletion of articles

8956ebc

ref: change soft_delete! to soft_delete

09e8c50

Deletion of subscriptions controller

a27935a

comment unusefull tests & try to fix those that did not passed

97ffc49

Comment outdated tests, fix existing that should pass

adbc98c

DioFun and others added 9 commits July 5, 2024 11:29

fix: add locals

ffbccc5

fix: add locals

6681798

test: add some test and fix problems related

ca250bd

test: add more tests

66907ce

fix: let rails issue errors when trying to destroy a dependant article

b64f9ec

feat: manage subscription offers with tests impleted

0e731a6

fix: mistake

6d236ec

wip: sales implementation

6ba8bd1

wip: dynamically add articles to a sale using StimulusJS

85f996d

DioFun self-assigned this Jul 5, 2024

D0gmaDev and others added 2 commits July 7, 2024 17:41

wip: generate invoice pdf with hexapdf

4d14f17

wip: add subscription to sale

518d6c6

DioFun requested a review from nymous July 8, 2024 22:38

DioFun added 2 commits July 9, 2024 00:40

fix(test): update fixtures regarding the column renaming of the last …

152a8b5

…commit

feat(paymentMethods): add, remove payment methods

2d9083e

fix(settingsFixture): add suitable parameters

b9db1cd

Letiste reviewed Jul 16, 2024

View reviewed changes

nymous reviewed Jul 16, 2024

View reviewed changes

DioFun added 4 commits July 16, 2024 11:26

refactor(sales): add default_scope for article, subscription_offer an…

d2391e9

…d payment_method

fix(tests): fix tests broke by last commit & advised change in review

e112313

fix(saleView): add min on duration field

4e2e840

test: add a test for soft deletion & check deleted_at not nil for sof…

fee752c

…t deletion tests

DioFun mentioned this pull request Jul 18, 2024

Todo list #435

Open

23 tasks

DioFun and others added 18 commits July 19, 2024 07:27

fix(sales): fix sales reation

06ad62e

refactor(invoiceSaleCreation): change name and place of the invoice h…

12e32fd

…ash generation for sale

feat(subscriptions): create subscription on sale creation

6cae8a0

fix(invoices): multiple pages pdf

012f6bd

fix(setting): rename last_invoice_id key to next_invoice_id

5b1bff1

refactor(sales): improve readability

8a5a714

test: write some invoice tests

6d3196f

test(invoices): write generator service tests

74232b0

test(articles_sales): create test for articles_sale model

ef231da

fix(sales): ensure duration is positive

a85b030

test(sales): start sales_controller tests

d7bbf2c

test(sales_controller): test if sales is not processable and duration…

604b731

… is negative

test(sales_controller): create test for invalid payment_method

470c527

fix(subscription): comment out cancel method

1768f59

test(sale): add test for sale model

e5b05ec

fix(subscription): comment out unuseful methods

8894741

fix(sale): generate_sales_subscription_offers now returns true

720c7d2

test(sale): test if subscription offers are not exhaustive

1a799a5

DioFun marked this pull request as ready for review August 3, 2024 09:36

DioFun requested review from nymous and Letiste August 3, 2024 09:37

Letiste reviewed Aug 10, 2024

View reviewed changes

	@article.soft_delete unless @article.destroy
	@article.destroy or @article.soft_delete

	@payment_method.soft_delete unless @payment_method.destroy
	@payment_method.destroy or @payment_method.soft_delete

	@subscription_offer.soft_delete unless @subscription_offer.destroy
	@subscription_offer.destroy or @subscription_offer.soft_delete

		// newArticle.getElementById("sale_article_id_new").id = `sale_article_id_${this.nextId}`
		// newArticle.getElementById("sale_quantity_new").id = `sale_quantity_${this.nextId}`

wip: paid subscriptions system and articles #467

Are you sure you want to change the base?

wip: paid subscriptions system and articles #467

Conversation

DioFun commented Jul 4, 2024 • edited Loading

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

Letiste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nymous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DioFun commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading