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

added draft lastschriftmandat #179

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

added draft lastschriftmandat #179

wants to merge 2 commits into from

Conversation

The-Ludwig
Copy link
Contributor

@The-Ludwig The-Ludwig commented Jan 22, 2023

Added basic mechanism to add a sepa lastschriftmandat to the db.
Still a little rough around the edges, so I need more help on this. Tests are still completely misssing...

Fixes #114

@codecov-commenter
Copy link

Codecov Report

Merging #179 (0a147f1) into master (c3b15ca) will decrease coverage by 5.26%.
The diff coverage is 49.15%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
- Coverage   75.46%   70.21%   -5.26%     
==========================================
  Files          23       24       +1     
  Lines        1011     1155     +144     
==========================================
+ Hits          763      811      +48     
- Misses        248      344      +96     
Impacted Files Coverage Δ
member_database/main.py 52.94% <34.21%> (-14.10%) ⬇️
member_database/forms.py 60.00% <49.20%> (-36.78%) ⬇️
member_database/models/sepa.py 61.29% <61.29%> (ø)
member_database/admin_views.py 84.09% <93.10%> (+0.75%) ⬆️
member_database/models/__init__.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@maxnoe
Copy link
Member

maxnoe commented Jan 23, 2023

Can you open a single PR blacking the whole project before proceeding? That would make reviewing the rest much easier

@The-Ludwig
Copy link
Contributor Author

I'd also vote for making black the official Code-Style and adding pre-commit hooks for that :-)

@The-Ludwig
Copy link
Contributor Author

Can you open a single PR blacking the whole project before proceeding? That would make reviewing the rest much easier

#181

creation_date = db.Column(db.DateTime(timezone=True), nullable=False)

@validates("iban")
def validate_iban(self, key, iban):
Copy link
Member

Choose a reason for hiding this comment

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

mmh, don't roll your own? Sure we need to implement this ourselves...? This should definitely be tested

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 we need to test this, generally this PR needs tests.
(I did not fully write this myself, though, I tuned an implementation I found on GitHub)

# ### end Alembic commands ###


def downgrade():
Copy link
Member

Choose a reason for hiding this comment

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

This should not be allowed ;)

Copy link
Member

Choose a reason for hiding this comment

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

It is possible in alembic to not implement the downgrade. Since this might remove data we need to keep, maybe this would be a point where we don't allow the downgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEPA Mandate
3 participants