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

Invoice Form and Payments #387

Merged
merged 21 commits into from
Sep 18, 2024
Merged

Invoice Form and Payments #387

merged 21 commits into from
Sep 18, 2024

Conversation

pxwxnvermx
Copy link
Contributor

@pxwxnvermx pxwxnvermx commented Sep 5, 2024

Network Manager Views

image image

Program Manager Views

image

Copy link
Collaborator

@calellowitz calellowitz left a comment

Choose a reason for hiding this comment

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

As usual tests would be great, but this looks really good.

}"
hx-post="{% url "opportunity:invoice_approve" org_slug=request.org.slug pk=opportunity.pk %}"
hx-swap="none"
hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}'>
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does this need a csrf token? shouldn't the django form handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Django was not able to find csrf token on forms submitted using 'hx-post'. This include the csrf token as a request header to provide the csrf token.

organization=opportunity.organization,
invoice=invoice,
)
payment.save()
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this create duplicates on multiple calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a filter to the query to only create payment for invoices that don't have a payment.

filter.addEventListener("change", (event) => {
const url = new URL(window.location);
url.searchParams.set("filter", event.target.value)
history.pushState(null, '', url);
Copy link
Member

Choose a reason for hiding this comment

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

Is this just to share selections via URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this updates the URL to have the filter selected by user.

</div>
<div class="modal-footer">
<button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Close</button>
<button type="submit" class="btn btn-primary">Save changes</button>
Copy link
Member

Choose a reason for hiding this comment

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

Missing translations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. da288f2

@sravfeyn
Copy link
Member

I have a ticket for which I am considering to create an accounting app (for creating Currency/exchange-rate logic), I remembered that the work in this PR could also fit well into that app, instead of the opportunity app. I will start an app, feel free to move these to that app.

Copy link
Collaborator

@calellowitz calellowitz left a comment

Choose a reason for hiding this comment

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

would still love tests but won't block

Base automatically changed from pkv/payment-accrual-report to main September 18, 2024 08:04
@pxwxnvermx
Copy link
Contributor Author

@calellowitz i will add tests for the PR in a subsequent PR once the changes are deployed.

@pxwxnvermx pxwxnvermx merged commit 65264b4 into main Sep 18, 2024
2 checks passed
@pxwxnvermx pxwxnvermx deleted the pkv/invoice-form-and-payments branch September 18, 2024 08:10
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.

4 participants