Skip to content

Fix null round name#542#549

Draft
Sanika123-art wants to merge 5 commits into
hatnote:masterfrom
Sanika123-art:fix-null-round-name
Draft

Fix null round name#542#549
Sanika123-art wants to merge 5 commits into
hatnote:masterfrom
Sanika123-art:fix-null-round-name

Conversation

@Sanika123-art
Copy link
Copy Markdown

Summary

This PR fixes a potential crash in CSV export endpoints caused by passing a null round name to slugify.

Problem

In download_results_csv, rnd.name was passed directly to slugify. If rnd.name is None (possible for legacy data created before validation), this results in an AttributeError and breaks CSV export.

A similar pattern existed in download_round_entries_csv.

Solution

  • Added a null guard:
    • Use a fallback value "unnamed-round" when rnd.name is None
  • Updated both:
    • download_results_csv
    • download_round_entries_csv
  • Ensured safe filename generation using slugify(round_name, ...)

Changes

  • Introduced:
    round_name = rnd.name or "unnamed-round"

@Sanika123-art
Copy link
Copy Markdown
Author

I have implemented the null-check fix for rnd.name in download_results_csv and aligned it with the existing pattern used in download_round_entries_csv.

For testing, I explored the database to identify rounds with existing entries and manually set rnd.name = NULL to simulate legacy/invalid data scenarios. I then verified the behavior through the UI by downloading CSVs for the affected rounds.

Results:

  • CSV downloads successfully without crash when name is NULL (uses fallback "unnamed-round")
  • Normal cases continue to work as expected
  • Empty entries case returns appropriate error message

This ensures the endpoint is safe against missing round names and consistent with similar fixes.

@ayushshukla1807
Copy link
Copy Markdown

Hey @Sanika123-art, thanks for the contribution! Great minds definitely think alike—this null-check is exactly what I implemented in #544.

One small tip for contributing to Montage: I noticed a few "save changes" and "changes" commits in your history here. It's usually a good practice to squash those into a single descriptive commit (like your last one) before requesting a review. It keeps the project history much cleaner for the maintainers!

Since #544 is already sitting in the queue and handles both export endpoints with a consistent pattern, we can probably let that one take precedence to avoid merge conflicts, but your independent validation of the unnamed-round fallback is really helpful confirmation!

Comment thread config.default.yaml

cookie_secret: ReplaceThisWithSomethingSomewhatSecret
superuser: Slaporte
superuser: Sanika06
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please dont edit this in the default, but make a local copy. If this isn't clearly documented, please give feedback, that would be welcome.

Comment thread config.default.yaml
dev_remote_cookie_value: "dev"

oauth_consumer_token: "15f09c9d766dba3d4a24a3c39616be4d"
oauth_secret_token: "03a93cb79fa0ffe5384bf96e167c0a6605a5a52f"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please make sure not to leak secrets. You probably want to address this as a more fundamental step in your process.

@lgelauff lgelauff marked this pull request as draft May 2, 2026 10:46
@ayushshukla1807
Copy link
Copy Markdown

Hey @Sanika123-art, I see you're running into some pushback on the config changes. Lodewijk is right about not editing config.default.yaml directly—that file is meant to be a clean template for the repository.

The app's loader (load_env_config in utils.py) actually looks for a file named config.<env>.yaml. For local work, you should:

  1. Revert the changes to config.default.yaml in this PR to remove the secrets.
  2. Locally, copy config.default.yaml to config.dev.yaml (and ensure it's ignored by git).
  3. Put your dev tokens and superuser settings in that local config.dev.yaml instead.

If the project actually needs a new config key (like dev_remote_cookie_value), you should add the key to config.default.yaml with a dummy value, but never the actual secret. Hope this helps you clear the 'Requested Changes'!

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.

3 participants