Schedule report tasks and add report preview view #1620
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GregKaleka
force-pushed
the
gk/issue-1603
branch
from
e64f108 to
d529a7a
Compare
daveoconnor
reviewed
Feb 7, 2025
| email = person_data.pop("email") or generate_fake_email( | ||
| f"{person_data['first_name']} {person_data['last_name']}" | ||
| ) | ||
| if not (user := User.objects.filter(email=email).first()): |
There was a problem hiding this comment.
nitpick: the brackets here probably aren't needed?
daveoconnor
reviewed
Feb 7, 2025
|
|
||
| return handled_commits | ||
|
|
||
|
|
||
| def check_credentials() -> list[str]: |
There was a problem hiding this comment.
Small thing to think about rather than a blocker. I'm not sure about the naming of this. I think it could be something more intuitive maybe. The first impression from the use later would be that check_credentials would return a bool. Maybe something like list_missing_credentials would be more intuitive?
There was a problem hiding this comment.
I'm not set on that name either if you can think of something better.
daveoconnor
reviewed
Feb 7, 2025
|
|
||
|
|
||
| @method_decorator(staff_member_required, name="get") | ||
| class ReportPreviewView(View): |
There was a problem hiding this comment.
Is it worth using the BoostVersionMixin here to have centralized handling of that for consistency.
daveoconnor
approved these changes
Feb 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/releases/<version_slug>/reportNote: I have not implemented PDF rendering.
I can do so, but wanted to have some discussion about what library to use. I've used WeasyPrint in the past, and the results have been good, though I recall the dev experience being annoying (someone on the team was always having to install some supporting thing on their machine).Update: I attempted to get PDF rendering working with weasyprint. The results were close but I could not get some of the styling to work. In agreement with Rob, we're going to move forward with this html-only version for now, since it takes a lot of the manual work out already. We can revisit programmatic PDF rendering in the future.
Fixes #1603