Refactor and improve invoices #1013

artur-intech · 2018-10-18T09:38:55Z

Changes highlights:

runner 'Invoice.cancel_overdue_invoices' in schedule.rb is changed to
rake 'invoices:cancel_overdue'.
invoices.payment_term database column is removed and its value is hardcoded in UI.
invoices.paid_at is removed as unused (@ratM1n reported zero rows with any value in prod as of 2019-03-11)
invoices.due_date column's type is now date.
Invoice#invoice_items renamed to Invoice#items and Invoice interface to get a list of items is unified.
Default date format in UI.
Default translations are used.
Tests improved.
Specs converted to tests and removed along with factories.
Database structure improved.

Deployment:

To deploy, rake data_migrations:populate_invoice_issue_date should be run after rake db:migrate. This task populates invoices.issue_date column with the date from invoices.created_at.

All invoices should be valid for a data migration to be able to run. @ratM1n Reported zero invalid invoices on prod on 2018-10-18, but it may change. To get a list of them run bundle exec rails c and insert the following: https://gist.github.com/artur-beljajev/c3303eb3b39dcedf4a721548d0f3e6b7

Verify:

Account top-up (total calculation)
Invoice cancellation (incl. automatic)
Payment-invoice matching
Directo integration
Changing days_to_keep_overdue_invoices_active setting in Admin portal

maciej-szlosarczyk

More general note: This is not a refactoring. There are changes to both the external interface (rake task) and database schema.

maciej-szlosarczyk · 2018-10-18T10:05:09Z

app/controllers/registrar/invoices_controller.rb

-        end_time = params[:q][:due_date_lteq].try(:to_date)
-        params[:q][:due_date_lteq] = end_time.try(:end_of_day)
+        end_time = params[:q][:due_date_lteq]
+        params[:q][:due_date_lteq] = end_time


I don't understand this without the additonal methods, how is it different than:

a = 1 b = a a = b # can be skipped

The rescue on lines 67-69 also seems weird now, the only error you might see is NoMethodError when q hash is Nil.

I postponed refactoring of that part of legacy code. There are some other things I don't understand as well.

maciej-szlosarczyk · 2018-10-18T10:08:28Z

db/structure.sql

@@ -1052,7 +1052,8 @@ CREATE TABLE public.invoices (
    cancelled_at timestamp without time zone,
    total numeric(10,2) NOT NULL,
    in_directo boolean DEFAULT false,
-    buyer_vat_no character varying
+    buyer_vat_no character varying,
+    date date


I am not sure if this is the right choice.

You need to know that it means the date the invoice was issued, and not due date, and it conflicts with SQL DATE keyword.

Indeed. Did I make some mistake somewhere so that you mentioned due date? :)

I agree that DATE itself is a reserved keyword, but in what way and what exactly conflicts?
I didn't notice any obstacles using it. Even if you need to use plain SQL, you just need to quote it.

Invoice has date so date seems like a natural, self-explanatory name of a column.

I agree that DATE itself is a reserved keyword, but in what way and what exactly conflicts?

It is a reserved keyword in all 3 recent SQL standards, which makes it generally not portable. In Oracle you cannot query such a column without escaping its name, it raises a parser exception.

I didn't notice any obstacles using it. Even if you need to use plain SQL, you just need to quote it.

That is precisely one of the reasons why you should avoid doing it. Quoted values are case sensitive, making them prone to bugs caused by typos and key rollovers.

create table dates(DATE DATE NOT NULL, ISSUE_DATE DATE NOT NULL); select * from dates; +--------+--------------+ | date | issue_date | |--------+--------------| +--------+--------------+ SELECT "Date" from dates; column "Date" does not exist LINE 1: SELECT "Date" from dates ^ HINT: Perhaps you meant to reference the column "dates.date". SELECT ISSUE_Date from dates; +--------------+ | issue_date | |--------------| +--------------+ SELECT 0

Invoice has date so date seems like a natural, self-explanatory name of a column.

It has 3 dates to be exact: the date when it was created, the date when it was sent to the customer and the date when it is due. Because of that, just date is not the best choice IMHO.

That is a contrived problem. We don't use Oracle. Very unlikely we will migrate to it in the visible future. Even if we need to, and plain SQL is needed, it's possible to either quote it or change the name to something else.

I still don't get the point... An invoice can have date, due_date, sent_date (or date_sent). What's the problem here?

Invoice date is quite standard terminology:
https://www.google.ee/search?q=invoice+date&rlz=1C1CHBF_enEE802EE802&source=lnms&tbm=isch&sa=X&ved=0ahUKEwiwg929l5DeAhXBZCwKHeCSBcUQ_AUIDigB&biw=1280&bih=584
https://en.wikipedia.org/wiki/Invoice

Do you mean it should be named issue_date because simply date is ambiguous or what?

Yes, that is what I mean.

OK, at least now I understand the point. However, I still see date as a better fit (it is short and concise). I bet nobody will ask you to clarify, if you say just "Invoice date".

@artur-beljajev please fix the reserved date keyword issue. You always fight against ambiguity so it is hard to understand why all of the sudden its OK even if it is error prone and raises specific problems as @maciej-szlosarczyk described.

I don't see a problem using date, which is a standard term for invoice.
https://www.google.ee/search?hl=en&as_q=&as_epq=invoice+date&as_oq=&as_eq=due&as_nlo=&as_nhi=&lr=&cr=&as_qdr=all&as_sitesearch=&as_occt=any&safe=images&as_filetype=&as_rights=
Omniva uses the same term: https://media.voog.com/0000/0042/1620/files/Estonian_e-invoice_description_ver1.2_eng.pdf
Our estonian_e_invoice gem also sticks to it.

I find invoice.date clearer and shorter. 99.9% of time we will use application layer (i.e. call invoice.date), instead of manually building queries (in which case you will need to quote it). I don't find invoice.date ambiguous.

Please elaborate, what problem it introduces in our case?

Another possible solution is to use alias_attribute :date, :issue_date in Invoice model, and rename database column invoices.date to invoices.issue_date.

maciej-szlosarczyk · 2018-10-18T10:09:35Z

lib/tasks/data_migrations/populate_invoice_date.rake

+      end
+    end
+
+    puts "Invoice processed: #{processed_invoice_count}"


Minor typo: Invoices processed, plural

artur-intech · 2018-10-18T14:27:44Z

More general note: This is not a refactoring. There are changes to both the external interface (rake task) and database schema.

It depends what you mean by external. The main point is that this PR does not (should not) affect any functional behavior (and UI).

vohmar · 2018-11-29T12:43:14Z

has merge conflicts with master

vohmar · 2018-12-03T16:11:33Z

It appears that the change to start using reserved keyword date is still in effect here. So this change is postponed until the conflict is resolved. Wil come back to this after auction center release.

artur-intech · 2019-03-09T14:49:56Z

@vohmar Is there anything preventing this PR from being merged with master? Invoice date issue has been fixed.

- `runner 'Invoice.cancel_overdue_invoices'` in `schedule.rb` is changed to `rake 'invoices:cancel_overdue'`. - `invoices.payment_term` database column is removed and its value is hardcoded in UI. - `invoices.paid_at` is removed as unused - `invoices.due_date` column's type is now `date`. - `Invoice#invoice_items` renamed to `Invoice#items` and `Invoice` interface to get a list of items is unified. - Default date format in UI. - Default translations are used. - Tests improved. - Specs converted to tests and removed along with factories. - Database structure improved.

maciej-szlosarczyk

Good stuff.

2 small suggestions that might simplify operating on dates, but nothing earth-shattering.

maciej-szlosarczyk · 2019-03-14T13:54:14Z

app/models/registrar.rb

-      due_date: (Time.zone.now.to_date + Setting.days_to_keep_invoices_active.days).end_of_day,
-      payment_term: 'prepayment',
+      issue_date: Time.zone.today,
+      due_date: (Time.zone.now + Setting.days_to_keep_invoices_active.days).to_date,


Time.zone.today + Setting.days_to_keep_invoices_active produces the same result, and does not convert between TimeWithZone and Date.

It was like this before. I will refactor it in a separate branch.

maciej-szlosarczyk · 2019-03-14T14:00:59Z

app/services/overdue_invoice_canceller.rb

+    30.days
+  end
+
+  def self.delay


I guess by renaming this to delay_in_days we might get rid of operations on duration and treat them as integers, as does Date.

def self.default_delay_in_days 30 end def self.delay_in_days Setting.days_to_keep_overdue_invoices_active || default_delay end

The point was to just to avoid using _days suffix and to introduce an abstraction (a delay). It allows to keep it in any units, be it a second, minute or year.

The rationale is that you postpone invoice cancellation for some period (after invoice due date). Delaying it for some period (a delay) sounds fine for me. Delaying it for 5 (an integer) does not.

vohmar · 2019-03-15T12:20:08Z

cannot test automatic invoice cancellation and days_to_keep_overdue_invoices_active setting on staging. The error:

registry@staging:~/registry/current$ /bin/bash -l -c 'export PATH="$HOME/.rbenv/bin:$PATH";eval "$(rbenv init -)"; cd /home/registry/registry/current && RAILS_ENV=staging bin/bundle exec rake billing:cancel_overdue_invoices'
rake aborted!
Don't know how to build task 'billing:cancel_overdue_invoices' (see --tasks)
/home/registry/registry/releases/699/vendor/bundle/ruby/2.4.0/gems/rake-12.3.1/exe/rake:27:in `<top (required)>'
bin/bundle:3:in `load'
bin/bundle:3:in `<main>'
(See full trace by running task with --trace)

artur-intech · 2019-03-15T12:26:03Z

@vohmar See changelog in the description. billing:cancel_overdue_invoice is no more valid.

artur-intech · 2019-03-15T12:29:20Z

/home/registry/registry/releases/699/vendor/bundle/ruby/2.4.0/

@ratM1n Why we test against Ruby 2.4.0, whereas registry project requires 2.4.5? https://github.com/internetee/registry/blob/master/.ruby-version

I understand there are no API changes between them, but I would still run exactly the same version on every deployment. Updating it should be just a matter of running rbenv install.

artur-intech added the production data migration label Oct 18, 2018

maciej-szlosarczyk reviewed Oct 18, 2018

View reviewed changes

vohmar assigned artur-intech Oct 18, 2018

artur-intech force-pushed the refactor-invoices branch from 9ea1cf5 to 7d0a36f Compare October 18, 2018 14:11

artur-intech force-pushed the refactor-invoices branch 4 times, most recently from 218546f to 97bebeb Compare October 22, 2018 20:37

artur-intech assigned vohmar and unassigned artur-intech Oct 22, 2018

This was referenced Oct 26, 2018

Registry 329 #1023

Closed

Change invoices.issue_date to NOT NULL #1008

Closed

vohmar assigned artur-intech and unassigned vohmar Nov 29, 2018

artur-intech force-pushed the refactor-invoices branch 2 times, most recently from 88f061c to 7f464ab Compare November 30, 2018 15:45

artur-intech assigned vohmar and unassigned artur-intech Nov 30, 2018

vohmar added the blocked label Dec 3, 2018

artur-intech mentioned this pull request Mar 8, 2019

Convert specs to tests #1098

Merged

artur-intech force-pushed the refactor-invoices branch 4 times, most recently from 056570c to 106f5e2 Compare March 9, 2019 14:37

artur-intech force-pushed the refactor-invoices branch from 106f5e2 to 706854b Compare March 9, 2019 14:59

vohmar removed their assignment Mar 11, 2019

artur-intech assigned artur-intech and unassigned maciej-szlosarczyk Mar 11, 2019

artur-intech force-pushed the refactor-invoices branch from 6e85f37 to 72ce9d7 Compare March 11, 2019 13:46

vohmar removed the blocked label Mar 12, 2019

artur-intech force-pushed the refactor-invoices branch 8 times, most recently from 79c7501 to b051ee6 Compare March 14, 2019 12:58

Artur Beljajev added 2 commits March 14, 2019 15:39

Remove unused view

f9c6a8e

artur-intech force-pushed the refactor-invoices branch from b051ee6 to f9c6a8e Compare March 14, 2019 13:40

artur-intech assigned vohmar and unassigned artur-intech Mar 14, 2019

maciej-szlosarczyk approved these changes Mar 14, 2019

View reviewed changes

artur-intech changed the title ~~Refactor invoices~~ Refactor and improve invoices Mar 15, 2019

vohmar merged commit 8bcba4f into master Mar 15, 2019

artur-intech mentioned this pull request Mar 15, 2019

Change invoices.issue_date database column to NOT NULL #1107

Merged

vohmar deleted the refactor-invoices branch March 26, 2019 13:53

artur-intech mentioned this pull request May 9, 2019

Invoice#payment_term is not used #643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and improve invoices #1013

Refactor and improve invoices #1013

artur-intech commented Oct 18, 2018 •

edited

Loading

maciej-szlosarczyk left a comment

maciej-szlosarczyk Oct 18, 2018

artur-intech Oct 18, 2018 •

edited

Loading

maciej-szlosarczyk Oct 18, 2018

artur-intech Oct 18, 2018 •

edited

Loading

maciej-szlosarczyk Oct 18, 2018

artur-intech Oct 18, 2018 •

edited

Loading

artur-intech Oct 18, 2018

maciej-szlosarczyk Oct 18, 2018

artur-intech Oct 18, 2018 •

edited

Loading

vohmar Oct 19, 2018

artur-intech Oct 19, 2018 •

edited

Loading

artur-intech Oct 19, 2018 •

edited

Loading

maciej-szlosarczyk Oct 18, 2018

artur-intech Oct 18, 2018

artur-intech commented Oct 18, 2018 •

edited

Loading

vohmar commented Nov 29, 2018

vohmar commented Dec 3, 2018

artur-intech commented Mar 9, 2019

maciej-szlosarczyk left a comment

maciej-szlosarczyk Mar 14, 2019

artur-intech Mar 14, 2019

maciej-szlosarczyk Mar 14, 2019

artur-intech Mar 14, 2019 •

edited

Loading

vohmar commented Mar 15, 2019 •

edited

Loading

artur-intech commented Mar 15, 2019 •

edited

Loading

artur-intech commented Mar 15, 2019 •

edited

Loading

Refactor and improve invoices #1013

Refactor and improve invoices #1013

Conversation

artur-intech commented Oct 18, 2018 • edited Loading

Changes highlights:

Deployment:

Verify:

maciej-szlosarczyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artur-intech Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artur-intech Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artur-intech Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artur-intech Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artur-intech Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

artur-intech Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artur-intech commented Oct 18, 2018 • edited Loading

vohmar commented Nov 29, 2018

vohmar commented Dec 3, 2018

artur-intech commented Mar 9, 2019

maciej-szlosarczyk 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

artur-intech Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

vohmar commented Mar 15, 2019 • edited Loading

artur-intech commented Mar 15, 2019 • edited Loading

artur-intech commented Mar 15, 2019 • edited Loading

artur-intech commented Oct 18, 2018 •

edited

Loading

artur-intech Oct 18, 2018 •

edited

Loading

artur-intech Oct 18, 2018 •

edited

Loading

artur-intech Oct 18, 2018 •

edited

Loading

artur-intech Oct 18, 2018 •

edited

Loading

artur-intech Oct 19, 2018 •

edited

Loading

artur-intech Oct 19, 2018 •

edited

Loading

artur-intech commented Oct 18, 2018 •

edited

Loading

artur-intech Mar 14, 2019 •

edited

Loading

vohmar commented Mar 15, 2019 •

edited

Loading

artur-intech commented Mar 15, 2019 •

edited

Loading

artur-intech commented Mar 15, 2019 •

edited

Loading