Skip to content
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

Feature/tw automatic group order invoice generation #907

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
0fa696c
add groupo order invoice and relation to group order
Viehlieb Dec 23, 2021
817c680
add mailer relevant files, add pdf relevant files and confgurations f…
Viehlieb Dec 23, 2021
50017fe
add functionality in views and controllers for generation of group or…
Viehlieb Dec 23, 2021
eadfbd5
add locales
Viehlieb Dec 23, 2021
298a476
add tests (integration + model)
Viehlieb Dec 23, 2021
72fa7c1
add finally schema.rb
Viehlieb Dec 23, 2021
06eb56a
add tax_number, payment_method and automatic invoice options to be ed…
Viehlieb Dec 23, 2021
f592b27
fix tiny issue where flash alert not shown
Viehlieb Dec 24, 2021
f428450
left over todo for naming od pdf file
Viehlieb Dec 24, 2021
303f902
locales for prev commits
Viehlieb Dec 24, 2021
0967981
configure rubocop_todo to prevent errors
Viehlieb Dec 24, 2021
bafa163
fix rubocop errors
Viehlieb Dec 24, 2021
1de377c
add vat exempt option in payment_tab ~ refs #automatic_go_invoices
Viehlieb Jan 27, 2022
bddaebe
add option vor vat_exempt inoices ~ refs automatic go invoices
Viehlieb Jan 27, 2022
1fc5823
fix order.ordergroup nil pointer error ~ automatic go invoices
Viehlieb Jan 28, 2022
81daddf
fix nil pointer~ refs automatic go invoice
Viehlieb Feb 3, 2022
bb69c35
rubocop styling ~ refs automatic go invoice
Viehlieb Feb 3, 2022
81a5194
add vat exempt option in payment_tab ~ refs #automatic_go_invoices
Viehlieb Jan 27, 2022
07b058a
Merge branch 'feature/tw_automatic_group_order_invoice_generation' of…
Viehlieb Feb 3, 2022
bb3e049
accept changesfrom upstream and resolve merge conflict ~ automatic go…
Viehlieb Feb 3, 2022
b2882d8
Merge branch 'feature/tw_automatic_group_order_invoice_generation'
Viehlieb Mar 1, 2022
76be8fd
wip on entering individual date for invoices
Viehlieb Mar 29, 2022
3683758
wip on generating invoices with date
Viehlieb Mar 30, 2022
5a817a9
add datefield default values max value cannot be blank
Viehlieb Mar 30, 2022
3149e00
adapt tests for manually generating group order invoices for order
Viehlieb Apr 5, 2022
d37a752
Merge branch 'master' of github.com:foodcoops/foodsoft into feature/t…
Viehlieb Apr 5, 2022
aab1dca
fix behavior - when link is provided in article details not clickable…
Viehlieb Apr 5, 2022
f3914f3
solve hover problem for ordering articles
Viehlieb Apr 7, 2022
59fd8da
remove footer on group order invoices showing time.now
Viehlieb Apr 11, 2022
0539edc
remove unneccessary files in app/view/group_order_invoices
Viehlieb Sep 6, 2022
b6854bc
add authentication to group_order_invoices controller
Viehlieb Sep 6, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions app/controllers/group_order_invoices_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
class GroupOrderInvoicesController < ApplicationController
include Concerns::SendGroupOrderInvoicePdf

def show
@group_order_invoice = GroupOrderInvoice.find(params[:id])
if FoodsoftConfig[:contact][:tax_number]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't we show an invoice without an tax number?

respond_to do |format|
format.pdf do
send_group_order_invoice_pdf @group_order_invoice if FoodsoftConfig[:contact][:tax_number]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you check already for the tax_number in line 6. what's the purpose of the second check here?

end
end
else raise RecordInvalid
redirect_back fallback_location: root_path, notice: 'Something went wrong', :alert => I18n.t('errors.general_msg', :msg => "#{error} " + I18n.t('errors.check_tax_number'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • what kind of error do you want to catch here?
  • please use translations for user visible strings.
  • please do not use notice and alert at the same time
  • there is usually no need to use I18n.t and you could use just t in the controllers
  • please use newer syntax name: value instead of :name => value

end
end

def destroy
goi = GroupOrderInvoice.find(params[:id])
@order = goi.group_order.order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of @order here?

goi.destroy
respond_to do |format|
format.js
format.json { head :no_content }
end
end

def create
go = GroupOrder.find(params[:group_order])
@order = go.order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of @order here?

goi = GroupOrderInvoice.find_or_create_by!(group_order_id: go.id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why find_or_create in the create method?

respond_to do |format|
format.js
end
redirect_back fallback_location: root_path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you redirect here to root_path?

rescue => error
redirect_back fallback_location: root_path, notice: 'Something went wrong', :alert => I18n.t('errors.general_msg', :msg => error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comments form line 13 apply here too

end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a newline at the end of the file

1 change: 1 addition & 0 deletions app/models/group_order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class GroupOrder < ApplicationRecord
has_many :group_order_articles, :dependent => :destroy
has_many :order_articles, :through => :group_order_articles
has_one :financial_transaction
has_one :group_order_invoice
belongs_to :updated_by, optional: true, class_name: 'User', foreign_key: 'updated_by_user_id'

validates_presence_of :order_id
Expand Down
60 changes: 60 additions & 0 deletions app/models/group_order_invoice.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
class GroupOrderInvoice < ApplicationRecord

belongs_to :group_order
validates_presence_of :group_order
validates_uniqueness_of :invoice_number
validate :tax_number_set
after_initialize :init, unless: :persisted?

def generate_invoice_number(count)
trailing_number = count.to_s.rjust(4, '0')
unless GroupOrderInvoice.find_by(invoice_number: Time.now.strftime("%Y%m%d") + trailing_number)
Time.now.strftime("%Y%m%d") + trailing_number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • is there a way to make the format configureable?
  • please create a variable for values you use multiple times

else
generate_invoice_number(count.to_i + 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • recursive calling generate_invoice_number here is a very bad idea, since you create a database query all the time which is slow and blocks everything else
  • you could either store the invoice number somewhere in the database or find the last invoice via a single query to the database (something like SELECT invoice_number ... WHER invoice_number LIKE '...%' ORDER BY invoice_number DESC LIMIT 1)

end
end

def tax_number_set
if FoodsoftConfig[:contact][:tax_number].blank?
errors.add(:group_order_invoice, "Keine Steuernummer in FoodsoftConfig :contact gesetzt")
end
Comment on lines +18 to +20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method does not access the database and therefore should be removed from the model and move to a better location (BTW: there are user visible string which are not translated)

end

def init
self.invoice_number = generate_invoice_number(1) unless self.invoice_number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use a before_validation hook instead (see e.g. set_password in the User model)

self.invoice_date = Time.now unless self.invoice_date
self.payment_method = FoodsoftConfig[:group_order_invoices]&.[](:payment_method) || I18n.t('activerecord.attributes.group_order_invoice.payment_method') unless self.payment_method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the payment method stored in FoodsoftConfig?
please do not use translated strings in the model. depending on the locale of the current user different data would be used and stored in the database

end

def name
I18n.t('activerecord.attributes.group_order_invoice.name') + "_#{invoice_number}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the translation should be moved from the model to a controller, since the model should be language independent

end

def load_data_for_invoice
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you copy everything here? usually working with a GroupOrderInvoice instance should be the way to go.

invoice_data = {}
order = group_order.order
invoice_data[:supplier] = order.supplier.name
invoice_data[:ordergroup] = group_order.ordergroup
invoice_data[:group_order] = group_order
invoice_data[:invoice_number] = invoice_number
invoice_data[:invoice_date] = invoice_date
invoice_data[:tax_number] = FoodsoftConfig[:contact][:tax_number]
invoice_data[:payment_method] = payment_method
invoice_data[:order_articles] = {}
group_order.order_articles.each do |order_article|
# Get the result of last time ordering, if possible
goa = group_order.group_order_articles.detect { |goa| goa.order_article_id == order_article.id }

# Build hash with relevant data
invoice_data[:order_articles][order_article.id] = {
:price => order_article.article.fc_price,
:quantity => (goa ? goa.quantity : 0),
:total_price => (goa ? goa.total_price : 0),
:tax => order_article.article.tax
}
end
invoice_data

end
end
13 changes: 13 additions & 0 deletions db/migrate/20211208142719_create_group_order_invoices.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class CreateGroupOrderInvoices < ActiveRecord::Migration[5.2]
def change
create_table :group_order_invoices do |t|
t.integer :group_order_id
t.bigint :invoice_number, unique: true, limit: 8
t.date :invoice_date
t.string :payment_method

t.timestamps
end
add_index :group_order_invoices, :group_order_id, unique: true
end
end