fix default event live mode in talk #1170
Conversation
Reviewer's GuideThis PR systematically replaces the Entity relationship diagram for Event visibility changeerDiagram
EVENT {
bool live
string other_fields
}
Class diagram for updated Event model visibility attributeclassDiagram
class Event {
+live: bool
...other attributes...
}
Class diagram for updated event serializersclassDiagram
class EventSerializer {
+live: bool
...other fields...
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `talk/src/tests/orga/views/test_orga_views_event.py:311` </location>
<code_context>
@pytest.mark.django_db
-def test_toggle_event_is_public_without_warnings(
+def test_toggle_event_live_without_warnings(
event, orga_client, default_submission_type, question, submission_type
):
</code_context>
<issue_to_address>
**suggestion (testing):** Test could assert that no warning messages are present after activation.
Add an explicit assertion to verify that the response contains no warning or error messages, clarifying the test's intent.
</issue_to_address>
### Comment 2
<location> `talk/src/tests/api/test_api_events.py:66-69` </location>
<code_context>
@pytest.mark.django_db
-def test_orga_can_see_nonpublic_events(client, event, other_event, orga_user_token):
- event.is_public = False
+def test_orga_can_see_nonlive_events(client, event, other_event, orga_user_token):
+ event.live = False
event.save()
</code_context>
<issue_to_address>
**suggestion (testing):** Test could assert that orga users see both live and non-live events explicitly.
Add assertions to confirm that both live and non-live events appear in the response, with their 'live' status accurately shown.
```suggestion
event.live = False
event.save()
other_event.live = True
other_event.save()
response = client.get(event.api_urls.base, follow=True)
assert response.status_code == 200
data = response.json()
event_ids = [e["id"] for e in data]
# Check both events are present
assert event.id in event_ids
assert other_event.id in event_ids
# Check their 'live' status
for e in data:
if e["id"] == event.id:
assert e["live"] is False
if e["id"] == other_event.id:
assert e["live"] is True
```
</issue_to_address>
### Comment 3
<location> `talk/src/tests/api/test_api_access_code.py:28` </location>
<code_context>
[email protected]("is_public", (True, False))
[email protected]("live", (True, False))
@pytest.mark.django_db
-def test_cannot_see_access_codes(client, event, is_public):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for orga or privileged users accessing access codes for non-live events.
The current test only checks anonymous access. Please add tests for orga or privileged users to confirm correct permission handling for non-live events.
Suggested implementation:
```python
@pytest.mark.parametrize("live", (True, False))
@pytest.mark.django_db
def test_cannot_see_access_codes(client, event, live):
with scope(event=event):
event.live = live
event.save()
response = client.get(event.api_urls.access_codes, follow=True)
assert response.status_code == 401
@pytest.mark.parametrize("user_type", ("orga", "privileged"))
@pytest.mark.django_db
def test_privileged_user_access_codes_for_non_live_event(request, event, user_type):
# Assume there are fixtures or helper functions to get orga/privileged clients
client = request.getfixturevalue(f"{user_type}_client")
with scope(event=event):
event.live = False
event.save()
response = client.get(event.api_urls.access_codes, follow=True)
# Adjust the expected status code as per your permission logic (403 or 200)
assert response.status_code in (200, 403)
```
- Ensure you have fixtures named `orga_client` and `privileged_client` available in your test suite. If not, you will need to implement them to provide authenticated clients for orga and privileged users.
- Adjust the expected status code in the assertion (`200` or `403`) to match your application's permission logic for orga/privileged users accessing access codes for non-live events.
</issue_to_address>
### Comment 4
<location> `talk/src/tests/api/test_api_mail.py:25` </location>
<code_context>
[email protected]("is_public", (True, False))
[email protected]("live", (True, False))
@pytest.mark.django_db
-def test_cannot_see_access_codes(client, event, is_public):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing mail template visibility for orga users on non-live events.
Add a test to confirm orga users can view mail templates for non-live events if this is expected behavior.
</issue_to_address>
### Comment 5
<location> `talk/src/tests/api/test_api_speaker_information.py:39` </location>
<code_context>
[email protected]("is_public", (True, False))
[email protected]("live", (True, False))
@pytest.mark.django_db
-def test_cannot_see_access_codes(client, event, is_public):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing speaker information visibility for orga users on non-live events.
</issue_to_address>
### Comment 6
<location> `app/eventyay/orga/views/event.py:190` </location>
<code_context>
def post(self, request, *args, **kwargs):
event = request.event
action = request.POST.get('action')
if action == 'activate':
if event.live:
messages.success(request, _('This event was already live.'))
else:
responses = activate_event.send_robust(event, request=request)
exceptions = [response[1] for response in responses if isinstance(response[1], Exception)]
if exceptions:
from eventyay.base.templatetags.rich_text import render_markdown
messages.error(
request,
mark_safe('\n'.join(render_markdown(e) for e in exceptions)),
)
else:
event.live = True
event.save()
event.log_action(
'eventyay.event.activate',
person=self.request.user,
orga=True,
data={},
)
messages.success(request, _('This event is now public.'))
for response in responses:
if isinstance(response[1], str):
messages.success(request, response[1])
else: # action == 'deactivate'
if not event.live:
messages.success(request, _('This event was already hidden.'))
else:
event.live = False
event.save()
event.log_action(
'eventyay.event.deactivate',
person=self.request.user,
orga=True,
data={},
)
messages.success(request, _('This event is now hidden.'))
return redirect(event.orga_urls.base)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Merge else clause's nested if statement into elif ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| @pytest.mark.django_db | ||
| def test_toggle_event_is_public_without_warnings( | ||
| def test_toggle_event_live_without_warnings( |
There was a problem hiding this comment.
suggestion (testing): Test could assert that no warning messages are present after activation.
Add an explicit assertion to verify that the response contains no warning or error messages, clarifying the test's intent.
|
|
||
|
|
||
| @pytest.mark.parametrize("is_public", (True, False)) | ||
| @pytest.mark.parametrize("live", (True, False)) |
There was a problem hiding this comment.
suggestion (testing): Consider testing speaker information visibility for orga users on non-live events.
| def post(self, request, *args, **kwargs): | ||
| event = request.event | ||
| action = request.POST.get('action') | ||
| if action == 'activate': |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Merge else clause's nested if statement into elif (
merge-else-if-into-elif) - Use named expression to simplify assignment and conditional (
use-named-expression) - Swap if/else branches (
swap-if-else-branches)
|
This PR partially fixes #984
so my suggestion is that we will have a new event model field named |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to replace uses of the is_public field with the live field for determining event visibility. The changes systematically update test files, view logic, templates, permissions, and API serializers to use event.live instead of event.is_public when checking whether an event should be publicly accessible.
Key Changes
- Updated all test fixtures and assertions to use
event.liveinstead ofevent.is_public - Modified permissions and business logic across views, middleware, and context processors to check
livestatus - Updated API serializers to expose
livefield instead ofis_publicin event list responses - Renamed test functions and parameters to reflect the new
liveterminology
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Multiple test files | Updated test fixtures and assertions from is_public to live |
| app/eventyay/orga/views/event.py | Changed event activation/deactivation logic to use live field |
| app/eventyay/api/serializers/event.py | Updated serializer to expose live instead of is_public |
| app/eventyay/talk_rules/*.py | Updated permission rules to check live status |
| app/eventyay/agenda/permissions.py | Modified visibility checks to use live field |
| Multiple template files | Updated conditional rendering to check event.live |
| app/eventyay/common/middleware/domains.py | Changed custom domain logic to filter by live |
| app/eventyay/cfp/views/auth.py | Updated access blocking to check live status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Based on #984:
If so, I propose to add these fields to
class EventMode(TextChoices):
OFFLINE = 'o'
TEST = 't'
LIVE = 'l'
The later two fields are boolean field because boolean type has two values. @mariobehling I wonder if |
|
Yes, live sounds correct. |
In future, we will give an option to enable ticketing component later. How do we handle event 'live' if ticket component is created later?
Not really when only Talk component is created independently and ticket component is created later or not created at all. |
|
TLDR; The Talk component can only go live if the Tickets component is already live. However, if the Tickets component fails to go live (for example, due to missing quotas or unconfigured payments), the Talk component cannot go live either. My understanding is that an event can be initially created with either the Talk or Ticket component, or with both simultaneously.
Given that, it’s not entirely clear which component should act as the base. For example, Talk and Video are independent, but both depend on the Ticket system in certain scenarios. To simplify this setup, I propose the following approach: Ticketing Component ConditionsFor the Ticketing component to go live, two conditions must be met:
Talk Component ConditionsThe Talk component does not have strict prerequisites for going live. The system automatically creates a default Possible Scenarios
Model ChangesWe introduce three new fields in the Event model:
These fields update automatically when the user enables or disables components. Logic Overviewdef enable_ticket_component():
if self.is_ticket_created_earlier:
# Verify quota and payment setup
# If missing, mark event as offline
self.live = False
else:
self.is_ticket_created_earlier = True
self.is_tickets_enabled = True
self._build_initial_ticket_data() # Defaults to live=False on first setup
def enable_talk_component():
# No strict conditions required for activation
pass
def take_event_live():
if self.is_tickets_enabled or (self.is_tickets_enabled and self.is_talk_enabled):
# Require quota and payment setup before going live
elif self.is_talk_enabled and not self.is_tickets_enabled:
# Allow going live (Talk has minimal default setup)
|
|
I think we need to define more clearly what “go live” means versus “is published.” We are running into conceptual and practical issues when “live” is tied too closely to the Tickets component. As mentioned in issue #1014, the root page of an event should, in future, always display the basic event information. Although this data technically resides in the Tickets component, it should be treated as part of a common layer of the event. This has several implications:
To achieve this separation, we may need an additional status field, e.g., Key question: How do we implement this distinction so that the event’s root page can be published (or unpublished) and is accessible (or inaccessible) even when the Tickets component is not live? |
When you referred root page of an event, is it /common/event/<org_slug>/<event_slug> or presale page where currently user buys tickets from? |
|
This is an example URL of a root page https://next.eventyay.com/a-team/summit/ 
|
|
I think that the published_at = DateTimeField(null=True) # For the `Event` itself.
is_tickets_live = BooleanField(default=False) # For Ticket subsystem
is_talk_live = BooleanField(default=False) # For Talk subsystem
is_video_live = BooleanField(default=False) # For video subsystemThe |
|
If an event is not live, it means it is still in draft status and ticket sales are not yet possible — no one can place orders or access the checkout. When an event is live but in test mode, the event is publicly visible, allowing organizers or testers to simulate the full purchasing process. However, all resulting orders are test orders that do not count as real sales and can be freely deleted once testing is complete. In contrast, once an event is live and in normal mode, all ticket sales are real transactions — they can no longer be deleted, only canceled or refunded according to the organizer’s policies. |
|
Thanks. Then the tickets subsystem will have 3 states. Instead of class TicketingMode(TextChoices):
OFFLINE = 'o', 'Offline'
TEST = 't', 'Test'
LIVE = 'l', 'Live'
ticketing_mode = CharField(max_length=1, choices=TicketingMode.choices, default=TicketingMode.OFFLINE)
|
3 participants
fixes: #984
Summary by Sourcery
Switch event visibility from the deprecated is_public flag to a new live flag and ensure it is used consistently as the default live mode across the Talk codebase
Bug Fixes:
Enhancements:
Tests: