Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Expand and enforce Cancel Interim Meeting Request comments length to 512 #7069

Merged
merged 2 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ietf/meeting/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,8 @@ def save(self, *args, **kwargs):
class InterimCancelForm(forms.Form):
group = forms.CharField(max_length=255, required=False)
date = forms.DateField(required=False)
comments = forms.CharField(required=False, widget=forms.Textarea(attrs={'placeholder': 'enter optional comments here'}), strip=False)
# max_length must match Session.agenda_note
comments = forms.CharField(max_length=512, required=False, widget=forms.Textarea(attrs={'placeholder': 'enter optional comments here'}), strip=False)

def __init__(self, *args, **kwargs):
super(InterimCancelForm, self).__init__(*args, **kwargs)
Expand Down
18 changes: 18 additions & 0 deletions ietf/meeting/migrations/0005_alter_session_agenda_note.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Copyright The IETF Trust 2024, All Rights Reserved

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("meeting", "0004_session_chat_room"),
]

operations = [
migrations.AlterField(
model_name="session",
name="agenda_note",
field=models.CharField(blank=True, max_length=512),
),
]
Comment on lines +1 to +18
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: #7003 also has a 0005 migration. One of these will have to be updated to 0006, depending on merge order.

2 changes: 1 addition & 1 deletion ietf/meeting/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,7 @@ class Session(models.Model):
group = ForeignKey(Group) # The group type historically determined the session type. BOFs also need to be added as a group. Note that not all meeting requests have a natural group to associate with.
joint_with_groups = models.ManyToManyField(Group, related_name='sessions_joint_in',blank=True)
attendees = models.IntegerField(null=True, blank=True)
agenda_note = models.CharField(blank=True, max_length=255)
agenda_note = models.CharField(blank=True, max_length=512)
requested_duration = models.DurationField(default=datetime.timedelta(0))
comments = models.TextField(blank=True)
scheduled = models.DateTimeField(null=True, blank=True)
Expand Down
12 changes: 11 additions & 1 deletion ietf/meeting/tests_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5595,8 +5595,17 @@ def test_interim_request_cancel(self, mock):
self.assertEqual(r.status_code, 403)
self.assertFalse(mock.called, 'Should not cancel sessions if request rejected')

# test cancelling before announcement
# test with overly-long comments
comments += '0123456789abcdef'*32
self.client.login(username="marschairman", password="marschairman+password")
r = self.client.post(url, {'comments': comments})
self.assertEqual(r.status_code, 200)
q = PyQuery(r.content)
self.assertTrue(q('form .is-invalid'))
# truncate to max_length
comments = comments[:512]

# test cancelling before announcement
length_before = len(outbox)
r = self.client.post(url, {'comments': comments})
self.assertRedirects(r, urlreverse('ietf.meeting.views.upcoming'))
Expand All @@ -5618,6 +5627,7 @@ def test_interim_request_cancel(self, mock):
self.assertEqual(session.agenda_note, comments)
self.assertEqual(len(outbox), length_before + 1)
self.assertIn('Interim Meeting Cancelled', outbox[-1]['Subject'])
self.assertIn(comments, get_payload_text(outbox[-1]))
self.assertTrue(mock.called, 'Should cancel sessions if request handled')
self.assertCountEqual(mock.call_args[0][1], meeting.session_set.all())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ The {{ group.name }} ({{ group.acronym }}) {% if not meeting.city %}virtual {% e
interim meeting for {{ meeting.date|date:"Y-m-d" }} from {{ start_time|time:"H:i" }} to {{ end_time|time:"H:i" }} {{ meeting.time_zone }}
has been cancelled.

{{ meeting.session_set.0.agenda_note }}
{{ meeting.session_set.first.agenda_note }}
Copy link
Collaborator Author

@pselkirk pselkirk Feb 16, 2024

Choose a reason for hiding this comment

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

Note: I'm deeply uneasy about this change, since I have to assume it worked before, but in my test environment, generated email had the line:
** No value found for 'meeting.session_set.0.agenda_note' **'

Copy link
Member

@rjsparks rjsparks Feb 16, 2024

Choose a reason for hiding this comment

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

Why assume it worked before? There's no test that looks to see if the note actually made it into the email i think.
Please add one.

I verified that the cancellation that led to this issue had a agenda_note added, but the email that went out did not contain it.

{% endtimezone %}

Loading