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/745/create invoice job #845

Merged
merged 2 commits into from
Aug 24, 2024
Merged

Conversation

njaeggi
Copy link
Contributor

@njaeggi njaeggi commented Aug 15, 2024

No description provided.

@njaeggi njaeggi linked an issue Aug 15, 2024 that may be closed by this pull request
3 tasks
@njaeggi njaeggi force-pushed the feature/745/create-invoice-job branch from 0c515b2 to a5c28ce Compare August 15, 2024 13:39
@njaeggi
Copy link
Contributor Author

njaeggi commented Aug 15, 2024

As soon as #744 is merged, this PR needs small adjustments and rebase, then it is ready to review 🚀

@njaeggi njaeggi force-pushed the feature/745/create-invoice-job branch 5 times, most recently from 57673ca to 921de2e Compare August 21, 2024 14:47
@njaeggi njaeggi marked this pull request as ready for review August 22, 2024 09:35
@njaeggi njaeggi force-pushed the feature/745/create-invoice-job branch 3 times, most recently from 60d8c22 to ed69b88 Compare August 22, 2024 09:46
Copy link
Contributor

@amaierhofer amaierhofer left a comment

Choose a reason for hiding this comment

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

Bitte noch fehlende specs ergänzen und prüfen ob die verschieben von ExternalInvoice nach InvoiceForm sinn machen würde. Beim Job müssen noch die callbacks entsprechend spez verwendet werden

class CreateMembershipInvoiceJob < BaseJob
self.parameters = [:external_invoice_id, :date, :discount, :new_entry]

def initialize(external_invoice_id, date, discount, new_entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

params anpassen gemäss spec (new_entry und discount als kw args mit default values), super aufruf ergänze

app/controllers/people/membership_invoices_controller.rb Outdated Show resolved Hide resolved
app/controllers/people/membership_invoices_controller.rb Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ def create
invoice_form.attributes = invoice_form_params

if invoice_form.valid? && create_invoice
enqueue_membership_invoice_job
Copy link
Contributor

Choose a reason for hiding this comment

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

Hier fehlt noch der check membership_invoice.invoice? und das als fehlerhaft markiern (status: error, log entry erzeugen), das auch mit spec abdecken

end

def perform
if membership_invoice.is_a?(Invoices::Abacus::MembershipInvoice)
Copy link
Contributor

Choose a reason for hiding this comment

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

membership_invoice.invoice? behandeln analog controller

def external_invoice = @external_invoice ||= ExternalInvoice::SacMembership.new(person: person)

def enqueue_membership_invoice_job
membership_invoice = @external_invoice.build_membership_invoice(invoice_form.discount, invoice_form.new_entry, invoice_form.reference_date)
Copy link
Contributor

Choose a reason for hiding this comment

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

Das if statement mit dem type check find ich nicht gut, besser wäre die fehler dann direkt von der form zu holen, e.g.

member_invoice = invoice_form.build_membership_invoice
if member_invoice 
 enqueue_job
else 
 handle_error(invoice_form.membership_invoice_error)
end

app/jobs/create_membership_invoice_job.rb Outdated Show resolved Hide resolved
app/models/external_invoice/sac_membership.rb Show resolved Hide resolved
end

def active_memberships
[member.membership_from_role(sac_member.stammsektion_role)] +
Copy link
Contributor

Choose a reason for hiding this comment

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

werde hier wirklich die richtigen methoden verwendet, kenne das api nicht aber deckt sich nicht 1:1 mit spec, bitte nochmals prüfen und fehlende tests ergänzen

@@ -39,6 +39,19 @@ def title
I18n.t("invoices.sac_memberships.title", year: year)
end

def build_membership_invoice(discount, new_entry, reference_date)
@date = reference_date
Copy link
Contributor

Choose a reason for hiding this comment

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

Ist das ausreichend, damit via context und sac_member das stichdatum richtig berücksichtigt wird?

@amaierhofer amaierhofer force-pushed the feature/745/create-invoice-job branch from ed69b88 to 6dfc7b7 Compare August 24, 2024 15:02
@amaierhofer amaierhofer enabled auto-merge (squash) August 24, 2024 15:03
@amaierhofer amaierhofer merged commit e1d4738 into master Aug 24, 2024
9 checks passed
@amaierhofer amaierhofer deleted the feature/745/create-invoice-job branch August 24, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INVOICE: Rechnung für Mitglied erstellen - Background Job
2 participants