-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: Event Invoice Refactor #7279
feat: Event Invoice Refactor #7279
Conversation
This pull request introduces 1 alert when merging 7a90f4e into 98a7563 - view on LGTM.com new alerts:
|
app/models/event_invoice.py
Outdated
) | ||
send_notif_monthly_fee_payment( | ||
self.user, | ||
event.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'event'
app/models/event_invoice.py
Outdated
link = '{}/invoices/{}'.format(frontend_url, self.identifier) | ||
send_email_for_monthly_fee_payment( | ||
self.user.email, | ||
event.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'event'
app/models/event_invoice.py
Outdated
) | ||
|
||
return self.invoice_pdf_url | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line contains whitespace
app/models/event_invoice.py
Outdated
currency = self.event.payment_currency | ||
ticket_fee_object = db.session.query(TicketFees).filter_by(currency=currency).first() | ||
if not ticket_fee_object: | ||
logger.error('Ticket Fee not found for event id {id}'.format(id=event.id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'event'
app/models/event_invoice.py
Outdated
def generate_pdf(self): | ||
admin_info = get_settings() | ||
currency = self.event.payment_currency | ||
ticket_fee_object = db.session.query(TicketFees).filter_by(currency=currency).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (93 > 90 characters)
app/models/event_invoice.py
Outdated
@@ -43,26 +51,88 @@ class EventInvoice(SoftDeletionModel): | |||
stripe_token = db.Column(db.String) | |||
paypal_token = db.Column(db.String) | |||
status = db.Column(db.String, default='due') | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line contains whitespace
from app.models import db | ||
from app.models.base import SoftDeletionModel | ||
from app.models.order import Order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'app.models.order.Order' imported but unused
This pull request introduces 7 alerts when merging 887c1af into 98a7563 - view on LGTM.com new alerts:
|
app/models/event_invoice.py
Outdated
|
||
|
||
def get_new_id(): | ||
return get_new_identifier(EventInvoice) | ||
return 'I' + get_new_identifier(EventInvoice, length=8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black would make changes.
app/models/event.py
Outdated
@@ -8,7 +8,7 @@ | |||
from sqlalchemy import event | |||
from sqlalchemy.sql import func | |||
|
|||
from app.api.helpers.db import get_count | |||
from app.api.helpers.db import get_count, get_new_identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'app.api.helpers.db.get_count' imported but unused
@@ -351,6 +352,8 @@ def create_save_pdf( | |||
dir_path='/static/uploads/pdf/temp/', | |||
identifier=get_file_name(), | |||
upload_dir='static/media/', | |||
new_renderer=False, | |||
extra_identifiers={}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
This pull request introduces 10 alerts when merging 38d7cc1 into 459965b - view on LGTM.com new alerts:
|
def test_send_monthly_invoice(db): | ||
"""Method to test monthly invoices""" | ||
|
||
TicketFeesFactory(service_fee=10.23, maximum_fee=11) | ||
TicketFeesFactory(service_fee=10.23, maximum_fee=11, country='global') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black would make changes.
@@ -305,7 +305,7 @@ | |||
'subject': u'{date} - Monthly service fee invoice for {event_name}', | |||
'message': ( | |||
u"The total service fee for the ticket sales of {event_name} in the month of {date} is {amount}." | |||
+ u"<br/> That payment for the same has to be made in two weeks. <a href='{payment_url}'>Click here</a> to " | |||
+ u"<br/> That payment for the same has to be made in 30 days. <a href='{payment_url}'>Click here</a> to " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (118 > 90 characters)
@celery.task(base=RequestContextTask, name='send.monthly.event.invoice') | ||
def send_monthly_event_invoice(): | ||
events = Event.query.filter_by(deleted_at=None, state='published').all() | ||
events = Event.query.filter_by(deleted_at=None).filter(Event.owner != None).all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comparison to None should be 'if cond is not None:'
This pull request introduces 1 alert when merging e86d4e9 into 8be1450 - view on LGTM.com new alerts:
|
Fixes fossasia/open-event-frontend#5120
For fossasia/open-event-frontend#5127