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

Requested billtype changes #1302

Merged
merged 5 commits into from
Mar 31, 2024

Conversation

TomRoussel
Copy link
Contributor

@TomRoussel TomRoussel commented Mar 31, 2024

This PR contains the changes requested after PR #1290 was merged. It contains only pretty minor changes.
Specifically this PR contains the following:

  • Some tests contained leftover references to the "Transfer" type, these were removed. Also one test specifically checking the "Transfer" type has been completely removed.
  • The web.settle route contained a "FIXME" comment. Looking into it, I think it was previously erroneously copied from edit_bill in web.py:811. It doesn't need to check if the bill belongs to the requested project as this function creates that bill itself.
  • In one of the tests, "fromage" was removed. This erasure has been undone, and the vibes of the codebase have been improved.
  • A test was requested for the web.settle route, however this route is already being tested in budget_test.py:1309, test_settle_button. In this test, the settle route is called and checked. So no new test was added.

@zorun, I think that was everything you asked back in #1290? Let me know if there is anything I missed!

@TomRoussel TomRoussel marked this pull request as ready for review March 31, 2024 15:11
@zorun
Copy link
Collaborator

zorun commented Mar 31, 2024

Thanks, it all looks good! Indeed, I overlooked the test checking the new endpoint.

FYI, we should also convert this new endpoint to POST to avoid XSS issues, but let's move that to another thread (I have started doing the conversion)

@zorun zorun merged commit a5f83de into spiral-project:master Mar 31, 2024
21 checks passed
@TomRoussel TomRoussel deleted the billtype-requested-changes branch April 1, 2024 18:49
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.

2 participants