Default to include Wikimedia ID in speaker export#2683
Default to include Wikimedia ID in speaker export#2683hongquan wants to merge 12 commits intofossasia:devfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds Wikimedia username to speaker export with default enabled and performs small cleanup of string concatenations in configuration lambdas. Class diagram for updated speaker export form and Wikimedia username settingclassDiagram
class SpeakerExportForm {
+BooleanField include_wikimedia_username
+__init__(args, kwargs)
+questions
+export_field_names
+get_queryset
+_get_submission_ids_value(obj)
+_get_submission_titles_value(obj)
+_get_include_wikimedia_username_value(obj)
+_prepare_object_data(obj)
}
class AppSettings {
+bool include_wikimedia_username = True
+bool type
+BooleanField form_class
+BooleanField serializer_class
}
SpeakerExportForm --> AppSettings : reads_default_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new export field name
include_wikimedia_usernameis confusing since it contains a verb and the corresponding value accessor actually returnsobj.wikimedia_username; consider renaming the exported column (and its getter) to something likewikimedia_usernameto better reflect the data being exported. - The form field
include_wikimedia_usernameis hard-coded withinitial=Truewhile there is also a configuration default forinclude_wikimedia_username; consider wiring the form’s initial value to the configuration value so they stay in sync if the default changes in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new export field name `include_wikimedia_username` is confusing since it contains a verb and the corresponding value accessor actually returns `obj.wikimedia_username`; consider renaming the exported column (and its getter) to something like `wikimedia_username` to better reflect the data being exported.
- The form field `include_wikimedia_username` is hard-coded with `initial=True` while there is also a configuration default for `include_wikimedia_username`; consider wiring the form’s initial value to the configuration value so they stay in sync if the default changes in the future.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates export/config defaults so Wikimedia usernames can be included by default, primarily targeting speaker exports.
Changes:
- Add a new (default-enabled) checkbox to include Wikimedia username in the orga speaker CSV/JSON export.
- Flip the global default for the
include_wikimedia_usernameevent setting toTrue(affecting other areas that read this setting as well). - Minor import consolidation / small string-expression cleanup in settings definitions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
app/eventyay/orga/forms/speaker.py |
Adds a new exportable column/checkbox for Wikimedia username, defaulting to selected. |
app/eventyay/base/configurations/default_setting.py |
Changes the default value of include_wikimedia_username (and includes minor formatting/import adjustments). |
| }, | ||
| 'include_wikimedia_username': { | ||
| 'default': 'False', | ||
| 'default': 'True', |
There was a problem hiding this comment.
Changing the global default for the include_wikimedia_username event setting from False to True has broader effects than speaker export: it will enable Wikimedia username collection/display in checkout and include it in other exports (e.g. order list export) by default for new events. Please confirm this wider default change is intended; if the goal is only to default the speaker export column to included, this setting default should likely remain False.
| 'default': 'True', | |
| 'default': 'False', |
There was a problem hiding this comment.
enable Wikimedia username collection/display in checkout
So, this means the ticketing checkout would always have "wikimedia" enabled? The decision if it is enabled should depend on the ticketing configuration. As far as I understand though, if we have it enabled here, it must be enabled in the ticketing config as well or how?
There was a problem hiding this comment.
@mariobehling This PR makes it enabled at the same time for two places: Ticketing and Speaker export (Talk). These two are not linked yet.
There was a problem hiding this comment.
Ok, in that case there should be a helper text and a link to the other place, I think.
Enabling this option will also collect the Wikimedia username during ticketing, if ticketing is enabled, and include it in the checkout process. For more details, see [link to relevant section].
There was a problem hiding this comment.
Still Wikimedia should not be a default field for all events. E.g. we dont want it on eventyay.com. It is preferable for wikimedia.eventyay.com though.
So, what options are there to control this? Maybe we need a config option for the platform?
In particular events on eventyay.com should not include Wikimedia as a default.
…ongquan/eventyay-tickets into feature/default-to-export-wikimedia-id
| 'avatar_license', | ||
| 'submission_ids', | ||
| 'submission_titles', | ||
| 'wikimedia_username', | ||
| ] |
There was a problem hiding this comment.
export_field_names always includes wikimedia_username. If you decide to hide/disable Wikimedia IDs based on event.settings.include_wikimedia_username (consistent with checkout and order export), this list should also be conditional; otherwise ExportForm.export_fields/export_data can crash due to missing self.fields['wikimedia_username'] or unexpectedly allow exporting the value when the setting is off.
|
Yes, I will research more in the codebase to see:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
app/eventyay/base/configurations/default_setting.py:242
include_wikimedia_usernameis now enabled by default, but the form field only has a label. Given this setting affects checkout field visibility and exports (e.g. order list exporter checks it), please add a clearhelp_textexplaining the broader impact and where to configure related ticketing/export behavior so organizers don’t enable it unintentionally.
'include_wikimedia_username': {
'default': 'True',
'type': bool,
'form_class': forms.BooleanField,
'serializer_class': serializers.BooleanField,
'form_kwargs': dict(
label=_('Add the Wikimedia ID for users authenticated via Wikimedia'),
),
From a recent discussion, the settings is difficult to find, our customer may not be aware of it. So we should let it default to
True.Wikimedia user without email address will be asked for email address (we will turn off this feature later):
Speaker export (Talk) includes "wikimedia username"