Conversation
|
could you move the addition of the vendored javascript libraries into separate commit(s)? That makes the other changes easier to review. |
|
@fabianfreyer Commit split up. Please review |
fabianfreyer
left a comment
fabianfreyer
left a comment
There was a problem hiding this comment.
Code itself LGTM, modulo testing.
Some smaller, architectural notes: I'm not sure the account_management system really is the right place for this, as it's becoming a big, clunky, monolith. I'm guessing that started with integrating the registration logic, which was really a hack. Originally, this application was supposed to remain a small, slim LDAP-Backed OAuth provider with some administrative views for account management. Maybe a standalone service that authenticates against the OAuth provider and uses the appropriate scopes to verify group membership and access might be more appropriate and maintainable?
Given my other feedback is mostly just nitpicking and formalities, I'm gonna recommend merging this PR (possibly adressing my other comments - that's up to you, @JanLuca) and then at some later time separating out this and the registration logic into a separate application.
| file_path = db.Column(db.Text()) | ||
| comment = db.Column(db.Text()) | ||
|
|
||
| class Batch(db.Model): |
There was a problem hiding this comment.
Maybe a more descriptive name here might be better, e.g. MailBatch
| recipients = db.relationship("Recipient", secondary=RecipientToList, backref=db.backref('lists', lazy=True)) | ||
| comment = db.Column(db.Text()) | ||
|
|
||
| class Decision(db.Model): |
There was a problem hiding this comment.
What exactly does a Decision represent? Maybe we can find better naming for this class...
|
|
||
| {% block content %} | ||
| <div class="container"> | ||
| <h1>Beschlüsse</h1> |
There was a problem hiding this comment.
We should really start using proper i18n for this..., but that's not in scope for this PR ¯_(ツ)_/¯
app/stapf/helpers.py
Outdated
| msg.attach(batch.decision.filename, "application/pdf", fp.read()) | ||
| conn.send(msg) | ||
|
|
||
| msg = Message(batch.subject, recipients=[current_app.config['MAIL_STAPF']], |
There was a problem hiding this comment.
Why is this being commented out? Could this be hidden behind a config variable or a check if MAIL_STAPF is even set?
2 participants
No description provided.