Tournament Data Export + Tab Card Refactor #346
Conversation
This file contains hidden or 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
JoeyRubas
force-pushed
the
csv-export
branch
from
571a093 to
bd9911c
Compare
BenMusch
reviewed
Nov 29, 2024
Member
BenMusch
left a comment
BenMusch
left a comment
There was a problem hiding this comment.
Thanks for this PR! A couple comments in line. It's been a while since I've written python or django code so take any of my comments and examples with a grain of salt
The last time I ran this code was 1-2 laptops ago so will take me a day or two to set things up to test the behavior here
(Also: the code in basically any module here is pretty outdated and hack-y, so apologies if any of my review of this PR is me nit-picking code that was just copied over. Would love to take this as an opportunity to clean things up but feel free to let me know if/where I've left comments on code that's just being copied and we can de-prioritize those changes)
mittab/apps/tab/tab_card.py
Outdated
| all_tab_cards_data = {} | ||
|
|
||
| # Prefetch related data to reduce database queries | ||
| teams = Team.objects.prefetch_related( |
Member
There was a problem hiding this comment.
I think we wanna fetch judges here as well, lemme test locally to see how to do that
Member
Author
|
Resolved comments that were added, rest should have comment replies and usually an associated change |
Member
|
@JoeyRubas Can you fix the linting errors in circle CI? After that will run manually to confirm it works and then merge |
Member
Author
|
@BenMusch Resolved the linting errors and in extension of this discussion, moved all three exports to one url, and a dedicated page/form to prevent navbar clutter. Everything works well by my testing, should be ready for testing by others! Also going to flag one or two other design choices that might require discussion below. Edit: has to fix one more linting error, now resolved. |
JoeyRubas
commented
Jan 18, 2025
mittab/apps/tab/views.py
Outdated
| @permission_required("tab.tab_settings.can_change", login_url="/403/") | ||
| def generate_archive(request): | ||
| tournament_name = request.META["SERVER_NAME"].split(".")[0] | ||
| def export_tournament(request): |
Member
Author
There was a problem hiding this comment.
I migrated the data export views from team_views info views, because I wanted them in the same file as the existing XML/archive, and this seemed more natural (broad tournament data doesn't seem too tightly coupled with teams?). The drawback is they're now in a different file than the HTML tab cards.
JoeyRubas
commented
Jan 18, 2025
|
|
||
|
|
||
| class JSONDecimalEncoder(json.JSONEncoder): | ||
| def default(self, o): # pylint: disable=E0202 |
Member
Author
There was a problem hiding this comment.
As per this issue on the pylint github, it seems like the way json wants you to handle setting up this encoder is incompatable with pylint's preferred course of action - hence the comment
JoeyRubas
changed the title
JoeyRubas
changed the title
JoeyRubas
changed the title
JoeyRubas
changed the title
JoeyRubas
force-pushed
the
csv-export
branch
from
a9c2bae to
bc31a65
Compare
BenMusch
requested changes
Mar 8, 2025
Member
BenMusch
left a comment
BenMusch
left a comment
There was a problem hiding this comment.
some minor edge cases, I think this could use 1 more pass
JoeyRubas
force-pushed
the
master
branch
2 times, most recently
from
b9653d1 to
29c52e5
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new tournament export feature that allows administrators to download tournament data in JSON, CSV, and XML formats. The implementation refactors existing export logic into reusable modules and adds a user-friendly interface for selecting export formats.
- Adds a new export tournament interface with support for JSON, CSV, and XML formats
- Refactors tab card and XML archive generation into dedicated modules (
mittab/libs/data_export/) - Enhances storage abstraction to support configurable prefixes and suffixes for different export types
- Integrates automatic export when results are published
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| mittab/urls.py | Adds URL patterns for the new export tournament feature |
| mittab/templates/common/_form.html | Adds support for custom submit button labels |
| mittab/templates/base/_navigation.html | Adds "Export Tournament" link to navigation menu |
| mittab/libs/tests/views/test_exports.py | Comprehensive test coverage for all export formats including edge cases |
| mittab/libs/data_export/xml_archive.py | New module for XML (DebateXML) tournament archive export |
| mittab/libs/data_export/tab_card.py | New module for JSON/CSV tab card exports with optimized queries |
| mittab/libs/data_export/s3_connector.py | New module for automated results export to storage |
| mittab/libs/backup/storage.py | Refactors storage classes to accept configurable prefix/suffix parameters |
| mittab/apps/tab/views/views.py | Adds export views and refactors existing archive generation |
| mittab/apps/tab/views/team_views.py | Simplifies tab_card view by using extracted export logic |
| mittab/apps/tab/models.py | Adds helper method to get or create team codes |
| mittab/apps/tab/forms.py | Adds ExportFormatForm for format selection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_victor_label(victor_code, side): | ||
| side = 0 if side == GOV else 1 | ||
| victor_map = { | ||
| 0 : ("", ""), |
There was a problem hiding this comment.
Inconsistent spacing in dictionary. There's a missing space after the colon on line 59 (0 : ("", "")) while other entries have proper spacing. Change to 0: ("", "") for consistency.
Suggested change
| 0 : ("", ""), | |
| 0: ("", ""), |
| breaking_team.effective_seed = breaking_team.seed | ||
| breaking_team.save() | ||
|
|
||
| return round_obj |
There was a problem hiding this comment.
Missing blank line before class definition. According to PEP 8, there should be two blank lines before top-level class definitions. Add a blank line before class ExportFormatForm.
Suggested change
| return round_obj | |
| return round_obj |
Comment on lines
+511
to
+512
| def _get_tournament_name(request): | ||
| return request.META["SERVER_NAME"].split(".")[0] |
There was a problem hiding this comment.
Potential security issue: The tournament name is extracted from SERVER_NAME without validation and used directly in the Content-Disposition header. A malicious SERVER_NAME could potentially inject headers. Consider sanitizing the tournament name or using a safer approach to generate filenames, such as using only alphanumeric characters and hyphens.
| def __init__(self, prefix=BACKUP_PREFIX, suffix=SUFFIX): | ||
| if not BUCKET_NAME: | ||
| raise ValueError("Need bucket name for S3 storage") | ||
| if not BACKUP_PREFIX: |
There was a problem hiding this comment.
The validation still checks the global BACKUP_PREFIX constant instead of the instance's prefix parameter. This means validation will fail even when a valid prefix parameter is passed. Line 56 should check if not prefix: or if not self.prefix: instead of if not BACKUP_PREFIX:.
Suggested change
| if not BACKUP_PREFIX: | |
| if not prefix: |
| max_index = min(cur_round - 1, len(round_stats)) | ||
| for i in range(max_index): | ||
| # Don't fill in totals for incomplete rounds | ||
| if round_stats[i][6] == blank and len(round_stats)>=i+2 and blank not in round_stats[i + 1][:5]: |
There was a problem hiding this comment.
Missing space before the comparison operator. Should be len(round_stats) >= i+2 with spacing: len(round_stats) >= i + 2 for better readability and consistency with Python style guidelines.
Suggested change
| if round_stats[i][6] == blank and len(round_stats)>=i+2 and blank not in round_stats[i + 1][:5]: | |
| if round_stats[i][6] == blank and len(round_stats) >= i + 2 and blank not in round_stats[i + 1][:5]: |
| def tab_cards_csv(request, tournament_name): | ||
| response = HttpResponse(content_type="text/csv") | ||
| response["Content-Disposition"] = f"attachment; filename={tournament_name}.csv" | ||
| buffer = io.StringIO(newline="") |
There was a problem hiding this comment.
The io.StringIO is created with newline="" parameter which is not supported. The StringIO constructor doesn't accept a newline parameter - that's only for opening files. Remove the newline="" argument.
Suggested change
| buffer = io.StringIO(newline="") | |
| buffer = io.StringIO() |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Impliment CSV, JSON exports for tab card data. Some refactors of existing tab card gen