Port /press/speakerrequest/ to bedrock. Bug 940555. #1639

Merged
merged 1 commit into from Feb 24, 2014

Projects

None yet

3 participants

@jpetto
Member
jpetto commented Jan 30, 2014

No description provided.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/forms.py
+ 'aria-required': 'true',
+ 'placeholder': _lazy(u'http://www.my-event.com'),
+ }
+ ),
+ )
+ sr_event_date = forms.CharField(
+ required=True,
+ error_messages={
+ 'required': _lazy(u'Please provide a date.'),
+ },
+ widget=DateInput(
+ attrs={
+ 'class': 'required',
+ 'required': 'required',
+ 'aria-required': 'true',
+ 'placeholder': _lazy(u'2015-03-14'),
alexgibson
alexgibson Jan 30, 2014 Member

Perhaps it would be better to use a more generic placeholder (i.e. YYYY-MM-DD), given that there is also a note showing an example date format underneath the field?

alexgibson
alexgibson Jan 30, 2014 Member

Actually disregard my previous suggestion here, seems the placeholder attribute is not allowed on input[type=date], so it should actually be removed.

http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#date-state-%28type=date%29

jpetto
jpetto Jan 30, 2014 Member

The placeholder is a fallback for browsers that don't supply a native datepicker widget. It's ignored on supporting browsers, but is kind of a hack. Could maybe use JS to add the attribute if the browser doesn't support date inputs?

Since we do have the note though, might be best to just remove it entirely.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
@@ -0,0 +1,277 @@
+{% extends "base-resp.html" %}
+
+{% block page_title %}{{_('Speaker Request Form')}}{% endblock %}
+{% block body_id %}press_speaker_request{% endblock %}
+{% block body_class %}sand{% endblock %}
+
+{% block site_css %}
+ {{ css('press_speaker_request') }}
+{% endblock %}
+
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ {# L10n: The line break below is for visual formatting only #}
alexgibson
alexgibson Jan 30, 2014 Member

Remove this L10n note as there is no line break in the following string?

jpetto
jpetto Jan 30, 2014 Member

Got it. Good catch.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+{% block body_id %}press_speaker_request{% endblock %}
+{% block body_class %}sand{% endblock %}
+
+{% block site_css %}
+ {{ css('press_speaker_request') }}
+{% endblock %}
+
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ {# L10n: The line break below is for visual formatting only #}
+ <h1 class="title-shadow-box">{{_('Speaker request form')}}</h1>
+ </header><!-- /header -->
+
+ <div class="container">
alexgibson
alexgibson Jan 30, 2014 Member

Replace <div> with <section>?

jpetto
jpetto Jan 30, 2014 Member

Agreed.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ {# L10n: The line break below is for visual formatting only #}
+ <h1 class="title-shadow-box">{{_('Speaker request form')}}</h1>
+ </header><!-- /header -->
+
+ <div class="container">
+ <h2 class="secondary-title">{{ _('Please complete and submit the form below to request a speaker from Mozilla for your upcoming event.') }}</h2>
+ <div class="main-column">
+ <p class="callout">
+ {% trans url='https://bugzilla.mozilla.org/form.dev-engagement-event' %}
+ If you are hosting a developer conference or event and want someone from
+ Mozilla's Developer Evangelism team to speak, please fill out
+ <a href="{{ url }}">this form</a> instead.
alexgibson
alexgibson Jan 30, 2014 Member

This link text could be slightly more descriptive so it makes better sense out of context. Maybe re-phrase this paragraph?

jpetto
jpetto Jan 30, 2014 Member

Agreed the copy isn't the best. Never heard back from request originator, so went with a quick suggestion. I'll re-word with the rest of the updates.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+ {{ form.sr_event_name }}
+ </div>
+
+ <div class="field required">
+ {{ form.sr_event_url.errors }}
+ <label for="sr_event_url">{{ _('Event URL') }}</label>
+ {{ form.sr_event_url }}
+ </div>
+
+ <div class="fields-container">
+ <div class="field required">
+ {{ form.sr_event_date.errors }}
+ <label for="sr_event_date">{{ _('Date') }}</label>
+ {{ form.sr_event_date }}
+ <div class="field-note date-note">
+ {{ _('2015-03-14 or March 14, 2015') }}
alexgibson
alexgibson Jan 30, 2014 Member

Would it be better to stick to suggesting a single form of date format? (I'd probably go for the ISO). Thoughts?

jpetto
jpetto Jan 30, 2014 Member

No enforced date format, as above.

@alexgibson alexgibson commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+ <div class="field required">
+ {{ form.sr_event_date.errors }}
+ <label for="sr_event_date">{{ _('Date') }}</label>
+ {{ form.sr_event_date }}
+ <div class="field-note date-note">
+ {{ _('2015-03-14 or March 14, 2015') }}
+ </div>
+ </noscript>
+ </div>
+
+ <div class="field required">
+ {{ form.sr_event_time.errors }}
+ <label for="sr_event_time">{{ _('Time') }}</label>
+ {{ form.sr_event_time }}
+ <div class="field-note time-note">
+ {{ _('2:30 PM or 14:30') }}
alexgibson
alexgibson Jan 30, 2014 Member

Likewise, maybe stick to a single suggested format (24 hour?)

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
media/css/press/speaker-request.less
+ margin-left: 100px;
+}
+
+.container {
+ width: auto;
+}
+
+.callout {
+ background: #e4f3ff;
+ padding: 12px @baseLine;
+ .border-radius(4px);
+ box-shadow: inset 0 0 2px rgba(0,0,0,0.2);
+}
+
+fieldset {
+ margin-bottom: @baseLine*2;
alexgibson
alexgibson Jan 30, 2014 Member

Nit: space either side of *

jpetto
jpetto Jan 30, 2014 Member

Got it.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
media/css/press/speaker-request.less
+ width: auto;
+}
+
+.callout {
+ background: #e4f3ff;
+ padding: 12px @baseLine;
+ .border-radius(4px);
+ box-shadow: inset 0 0 2px rgba(0,0,0,0.2);
+}
+
+fieldset {
+ margin-bottom: @baseLine*2;
+}
+
+.field {
+ input[type=text], input[type=email], input[type=tel], textarea {
alexgibson
alexgibson Jan 30, 2014 Member

Change to using a multi-line rule for this selector (both here and below in the media queries).

jpetto
jpetto Jan 30, 2014 Member

Got it.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
media/css/press/speaker-request.less
+ }
+}
+
+.fields-container {
+ .clearfix;
+ .field {
+ float: left;
+ width: 210px;
+ margin-right: @baseLine;
+ input[type=text] {
+ width: auto;
+ }
+ }
+}
+
+.datepicker > .datepicker_inner_container > .datepicker_calendar > .datepicker_table > tbody > tr > th {
alexgibson
alexgibson Jan 30, 2014 Member

Redundant selector? (presumably the old version of this page used a date-picker widget?)

jpetto
jpetto Jan 30, 2014 Member

Ah, yep. Removing.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
media/js/press/speaker-request.js
@@ -0,0 +1,19 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+(function($, Modernizr) {
+ 'use strict';
alexgibson
alexgibson Jan 30, 2014 Member

Nit: use 4 space indentation for JS

jpetto
jpetto Jan 30, 2014 Member

Will try to remember this. Always think it's 2. Bah!

@alexgibson alexgibson commented on the diff Jan 30, 2014
bedrock/press/forms.py
+ },
+ widget=forms.TextInput(
+ attrs={
+ 'class': 'required',
+ 'required': 'required',
+ 'aria-required': 'true',
+ 'placeholder': _lazy(u'http://www.my-event.com'),
+ }
+ ),
+ )
+ sr_event_date = forms.CharField(
+ required=True,
+ error_messages={
+ 'required': _lazy(u'Please provide a date.'),
+ },
+ widget=DateInput(
alexgibson
alexgibson Jan 30, 2014 Member

Should this be forms.DateInput?

Without it, I notice jQuery validate does not seem to highlight the field when it's invalid:

validate

jpetto
jpetto Jan 30, 2014 Member

As the data is just going to an email (and not into a database or something), I'm not enforcing any particular date format here. Using forms.DateInput would make Django validate the format. All I really care about here is that data is entered.

This came after a bit of discussion and attempts at using a fallback JavaScript datepicker...which turns out to just be more work than it's worth. I think we can trust users to enter a decipherable date. On the off chance they don't, we have their email to contact them.

@alexgibson alexgibson commented on the diff Jan 30, 2014
bedrock/press/forms.py
+ },
+ widget=DateInput(
+ attrs={
+ 'class': 'required',
+ 'required': 'required',
+ 'aria-required': 'true',
+ 'placeholder': _lazy(u'2015-03-14'),
+ }
+ ),
+ )
+ sr_event_time = forms.CharField(
+ required=True,
+ error_messages={
+ 'required': _lazy(u'Please provide a time.'),
+ },
+ widget=TimeInput(
alexgibson
alexgibson Jan 30, 2014 Member

Should this be forms.TimeInput? There are also some other fields below where this is omitted, but I'm not sure if it's required.

jpetto
jpetto Jan 30, 2014 Member

Same issue as above with date - don't want to enforce any particular format. Want to go for the least amount of friction when filling out the form.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/forms.py
+ max_length=200,
+ required=False,
+ )
+ sr_event_goal = forms.CharField(
+ max_length=300,
+ required=False,
+ )
+ sr_event_format = forms.CharField(
+ max_length=200,
+ required=False,
+ )
+ sr_event_audience_size = forms.IntegerField(
+ required=False,
+ widget=NumberInput(
+ attrs={
+ 'min': '1',
alexgibson
alexgibson Jan 30, 2014 Member

Could use some placeholder text here to indicate a numeric value? Same could apply to the other number input below.

Seems to be valid for this input type: http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#number-state-%28type=number%29

jpetto
jpetto Jan 30, 2014 Member

Ah, good call. Will add a placeholder.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+ <label>{{ _('Type of Presentation') }}</label>
+ {{ form.sr_presentation_type }}
+ </div>
+
+ <div class="field">
+ {{ form.sr_presentation_topic.errors }}
+ <label for="sr_presentation_topic">{{ _('Topic of Presentation') }}</label>
+ {{ form.sr_presentation_topic }}
+ </div>
+
+ <div class="field">
+ {{ form.sr_presentation_length.errors }}
+ <label for="sr_presentation_length">{{ _('Expected Length') }}</label>
+ {{ form.sr_presentation_length }}
+ <div class="field-note">
+ ({{ _('in hours') }})
alexgibson
alexgibson Jan 30, 2014 Member

This note could be a bit more descriptive to indicate the input format, and also make clear you can enter half-hour values?

jpetto
jpetto Jan 30, 2014 Member

Will update.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+ {{ _('Maximum file size is 5 MB.') }}
+ </div>
+ </div>
+ </fieldset>
+
+ <button type="submit" name="submit_form" class="button">{{ _('Submit') }}</button>
+
+ {{ form.superpriority }}
+ </form>
+ {% else %}
+ <h2>{{ _('Your request has been sent.') }}</h2>
+
+ <p>{{ _('Thank you for your interest.') }}</p>
+ {% endif %}
+ </div><!--/.main-column-->
+ <aside class="sidebar">
alexgibson
alexgibson Jan 30, 2014 Member

Nit: line break after end of .main-column

jpetto
jpetto Jan 30, 2014 Member

Got it.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+{% block body_class %}sand{% endblock %}
+
+{% block site_css %}
+ {{ css('press_speaker_request') }}
+{% endblock %}
+
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ {# L10n: The line break below is for visual formatting only #}
+ <h1 class="title-shadow-box">{{_('Speaker request form')}}</h1>
+ </header><!-- /header -->
+
+ <div class="container">
+ <h2 class="secondary-title">{{ _('Please complete and submit the form below to request a speaker from Mozilla for your upcoming event.') }}</h2>
alexgibson
alexgibson Jan 30, 2014 Member

Currently you still get this title (and the developer evangelism note below) after the form is submitted. Perhaps move this content inside the conditional below, so the "Thank you" message can be a bit more prominent?

jpetto
jpetto Jan 30, 2014 Member

That would be better UI. Will fix.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/speaker-request.html
+
+ <div class="field required">
+ {{ form.sr_event_url.errors }}
+ <label for="sr_event_url">{{ _('Event URL') }}</label>
+ {{ form.sr_event_url }}
+ </div>
+
+ <div class="fields-container">
+ <div class="field required">
+ {{ form.sr_event_date.errors }}
+ <label for="sr_event_date">{{ _('Date') }}</label>
+ {{ form.sr_event_date }}
+ <div class="field-note date-note">
+ {{ _('2015-03-14 or March 14, 2015') }}
+ </div>
+ </noscript>
alexgibson
alexgibson Jan 30, 2014 Member

Remove orphaned </noscript>

jpetto
jpetto Jan 30, 2014 Member

Ack! Got it.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/forms.py
+ 'aria-required': 'true',
+ 'placeholder': _lazy(u'2015-03-14'),
+ }
+ ),
+ )
+ sr_event_time = forms.CharField(
+ required=True,
+ error_messages={
+ 'required': _lazy(u'Please provide a time.'),
+ },
+ widget=TimeInput(
+ attrs={
+ 'class': 'required',
+ 'required': 'required',
+ 'aria-required': 'true',
+ 'placeholder': _lazy(u'2:30 PM'),
jpetto
jpetto Jan 30, 2014 Member

Yeah, will remove as with date above.

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/forms.py
+ )
+ sr_event_media_coverage = forms.CharField(
+ max_length=500,
+ required=False,
+ widget=forms.Textarea(),
+ )
+ sr_event_sponsors = forms.CharField(
+ max_length=500,
+ required=False,
+ widget=forms.Textarea(),
+ )
+ sr_event_confirmation_deadline = forms.DateField(
+ required=False,
+ widget=DateInput(
+ attrs={
+ 'placeholder': _lazy(u'2015-03-14'),
alexgibson
alexgibson Jan 30, 2014 Member

Remove placeholder attribute.

jpetto
jpetto Jan 30, 2014 Member

Will do.

Member

@craigcook's suggestion of changing <div class="field"> to list items is probably worth consideration if you have time, but otherwise it's an r+ from me on the front-end.

@pmclanahan pmclanahan and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/forms.py
@@ -0,0 +1,252 @@
+# coding: utf-8
+
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from django import forms
+
+from lib.l10n_utils.dotlang import _, _lazy
+
+from bedrock.mozorg.forms import (HoneyPotWidget, EmailInput, DateInput,
pmclanahan
pmclanahan Jan 30, 2014 Owner

NIT: Alphabetize imports.

jpetto
jpetto Jan 31, 2014 Member

Roger.

@pmclanahan pmclanahan commented on the diff Jan 30, 2014
bedrock/press/forms.py
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from django import forms
+
+from lib.l10n_utils.dotlang import _, _lazy
+
+from bedrock.mozorg.forms import (HoneyPotWidget, EmailInput, DateInput,
+ TimeInput, TelInput, NumberInput, URLInput)
+
+
+SPEAKER_REQUEST_FILE_SIZE_LIMIT = 5242880 # 5MB
+
+
+class SpeakerRequestForm(forms.Form):
+ # event fields
+ sr_event_name = forms.CharField(
pmclanahan
pmclanahan Jan 30, 2014 Owner

why prefix field names with "sr_"?

jpetto
jpetto Jan 31, 2014 Member

Just to make sure they're different from the existing PHP page for spam bot protection.

pmclanahan
pmclanahan Jan 31, 2014 Owner

If you'd like to keep the field names more descriptive but also get the name differences you can instantiate the Form object in the view using the prefix argument. But since this is done, that's really just a nit. Just for future reference this works well :)

jpetto
jpetto Jan 31, 2014 Member

Aw man, that would have saved a bunch of time! Well, learning the hard way. 😄

@pmclanahan pmclanahan and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/emails/speaker-request.txt
+{{ sr_contact_name|safe|strip_tags }}
+
++ Title
+{{ sr_contact_title|safe|strip_tags }}
+
++ Company
+{{ sr_contact_company|safe|strip_tags }}
+
++ Phone
+{{ sr_contact_phone }}
+
++ Email
+{{ sr_contact_email }}
+
++ URL
+{{ sr_contact_company_url }}
pmclanahan
pmclanahan Jan 30, 2014 Owner

This may be overkill, but it might be nice to wrap some of the optional fields in {% if sr_contact_company_url %} blocks. Most emails would likely be much shorter in those cases.

jpetto
jpetto Jan 31, 2014 Member

Ah, that would make for a more focused email. Will do.

@pmclanahan pmclanahan and 1 other commented on an outdated diff Jan 30, 2014
bedrock/press/templates/press/emails/speaker-request.txt
++ Type of Presentation
+{{ sr_presentation_type }}
+
++ Other Panelists
+{{ sr_presentation_panelists|safe|strip_tags }}
+
++ Topic of Presentation
+{{ sr_presentation_topic|safe|strip_tags }}
+
++ Expected Length
+{{ sr_presentation_length }}
+
+------------------------------------
+
++ Attachment
+{{ sr_attachment }}
pmclanahan
pmclanahan Jan 30, 2014 Owner

Isn't this going to just be attached to the email?

jpetto
jpetto Jan 31, 2014 Member

D'oh, yep. Copy pasta from the PHP email template. Will remove.

@pmclanahan pmclanahan commented on an outdated diff Jan 31, 2014
bedrock/press/views.py
+
+from django.core.mail import EmailMessage
+from django.shortcuts import redirect
+from django.views.decorators.csrf import csrf_protect
+
+from funfactory.urlresolvers import reverse
+
+from forms import SpeakerRequestForm
+
+
+SPEAKER_REQUEST_EMAIL_FROM = 'Mozilla.com <noreply@mozilla.com>'
+SPEAKER_REQUEST_EMAIL_SUBJECT = 'New speaker request form submission'
+SPEAKER_REQUEST_EMAIL_TO = ['events@mozilla.com']
+
+
+def submit_form(request, form):
pmclanahan
pmclanahan Jan 31, 2014 Owner

Instead of having to do all the form stuff in this separate function and passing around all of the success flags and the form object, you could add a method to the form class to send the email. That would simplify the view to something more like:

if form.is_valid():
    form.send_mail()
else:
    # do other things
@pmclanahan pmclanahan commented on an outdated diff Jan 31, 2014
etc/httpd/global.conf
@@ -654,3 +654,6 @@ RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?projects/calendar(/?)$ /b/$1projects/calenda
# bug 947890
RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?firefox/latest/releasenotes(/?)$ /b/$1firefox/latest/releasenotes$2 [PT]
+
+# bug 940555
+RewriteRule ^/(\w{2,3}(?:-\w{2})?/)?press/speakerrequest/?$ /b/$1press/speakerrequest/ [PT]
pmclanahan
pmclanahan Jan 31, 2014 Owner

Should capture the optional / at the end and pass it to the proxied URL so that Django will redirect people to the version with the trailing /.

Member
jpetto commented Feb 5, 2014

I think code should now be complete. Waiting for final review from @pmclanahan, then will squash.

@pmclanahan pmclanahan commented on an outdated diff Feb 6, 2014
bedrock/press/tests.py
+ # make sure form is valid
+ ok_(form.is_valid())
+
+ def test_form_invalid_attachement(self):
+ """
+ Form should be invalid and contain attachment errors when attachment
+ over size limit.
+ """
+ # attachment within size limit
+ mock_attachment = Mock(
+ _size=(press_forms.SPEAKER_REQUEST_FILE_SIZE_LIMIT + 1))
+
+ form = SpeakerRequestForm(self.data, {'sr_attachment': mock_attachment})
+
+ # make sure form is not valid
+ eq_(False, form.is_valid())
pmclanahan
pmclanahan Feb 6, 2014 Owner

NIT: I think either self.assertFalse(form.is_valid()) or ok_(not form.is_valid()) makes more sense. This probably works, but technically is_valid() needs only be falsy, not specifically False.

@pmclanahan pmclanahan commented on an outdated diff Feb 6, 2014
bedrock/press/templates/press/speaker-request.html
+
+ <section class="container">
+ {% if not form_submitted or (form_submitted and form_error) %}
+ <h2 class="secondary-title">{{ _('Please complete and submit the form below to request a speaker from Mozilla for your upcoming event.') }}</h2>
+ {% endif %}
+ <div class="main-column">
+ {% if not form_submitted or (form_submitted and form_error) %}
+ <p class="callout">
+ {% trans url='https://bugzilla.mozilla.org/form.dev-engagement-event' %}
+ If you are hosting a developer conference or event and want someone from
+ Mozilla's developer team to speak, please fill out the
+ <a href="{{ url }}">Developer Events Request Form</a> instead.
+ {% endtrans %}
+ </p>
+
+ {% if form_error %}
pmclanahan
pmclanahan Feb 6, 2014 Owner

you can just use {% if form.errors %) and not have to add this to the context.

@pmclanahan pmclanahan commented on an outdated diff Feb 6, 2014
bedrock/press/templates/press/speaker-request.html
+{% block body_id %}press_speaker_request{% endblock %}
+{% block body_class %}sand{% endblock %}
+
+{% block site_css %}
+ {{ css('press_speaker_request') }}
+{% endblock %}
+
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ <h1 class="title-shadow-box">{{_('Speaker request form')}}</h1>
+ </header><!-- /header -->
+
+ <section class="container">
+ {% if not form_submitted or (form_submitted and form_error) %}
pmclanahan
pmclanahan Feb 6, 2014 Owner

Just set a form_success variable in the view.

pmclanahan
pmclanahan Feb 6, 2014 Owner

Alternately the easiest thing to do is to just have a separate thankyou template and page.

@pmclanahan pmclanahan commented on the diff Feb 6, 2014
bedrock/press/views.py
@@ -0,0 +1,49 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
pmclanahan
pmclanahan Feb 6, 2014 Owner

This view is confusing. I recommend letting Django help you with this by using the generic view class FormView. Here's how it should look (I think this should "Just Work"):

# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

from django.utils.decorators import method_decorator
from django.views.decorators.csrf import csrf_protect
from django.views.generic.edit import FormView

from funfactory.urlresolvers import reverse

from .forms import SpeakerRequestForm
from lib import l10n_utils


class SpeakerRequestView(FormView):
    form_class = SpeakerRequestForm
    success_url = reverse('press.speaker-request') + '?success=true'
    template_name = 'press/speaker-request.html'

    @method_decorator(csrf_protect)
    def dispatch(self, request, *args, **kwargs):
        return super(SpeakerRequestView, self).dispatch(request, *args, **kwargs)

    def get_form_kwargs(self):
        kwargs = super(SpeakerRequestView, self).get_form_kwargs()
        kwargs['auto_id'] = '%s'
        return kwargs

    def get_context_data(self, **kwargs):
        context = super(SpeakerRequestView, self).get_context_data(**kwargs)
        context['form_success'] = 'success' in self.request.GET
        return context

    def form_valid(self, form):
        form.send_email(self.request)
        return super(SpeakerRequestView, self).form_valid(form)

    def render_to_response(self, context, **response_kwargs):
        return l10n_utils.render(self.request,
                                 self.get_template_names(),
                                 context,
                                 **response_kwargs)
pmclanahan
pmclanahan Feb 6, 2014 Owner

Another bonus of the class-based view approach is that you could move the "send email" method back to the view as a method so you'd have access to self.request and not have to pass the request into the form method. But either way is fine with me.

jpetto
jpetto Feb 6, 2014 Member

I was already on board with the class-based view, but not having to pass around self.request is the icing on the 🍰. It worked, but felt very hacky. Will update either today or tomorrow.

pmclanahan
pmclanahan Feb 6, 2014 Owner

\o/ for CBVs!

Member
jpetto commented Feb 7, 2014

Class-based view is in place. Tests are passing. View code feels much better. @pmclanahan - (hopefully only) one more r?

Owner

Nice! First pass at the changes look really good. I want to look closer at the tests, but will have to continue later. I'll get back to you soon, but thanks for updating all of that. It's so much cleaner and easier to follow :)

Member
jpetto commented Feb 7, 2014

I'll be buried in MWC next week and will probably forget this PR exists. Take your time. 😄

Thank you for all the help.

@pmclanahan pmclanahan commented on an outdated diff Feb 10, 2014
bedrock/press/templates/press/speaker-request.html
+{% block body_id %}press_speaker_request{% endblock %}
+{% block body_class %}sand{% endblock %}
+
+{% block site_css %}
+ {{ css('press_speaker_request') }}
+{% endblock %}
+
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ <h1 class="title-shadow-box">{{_('Speaker request form')}}</h1>
+ </header><!-- /header -->
+
+ <section class="container">
+ {% if not form_success or form.errors %}
pmclanahan
pmclanahan Feb 10, 2014 Owner

Don't need the or form.errors here I think. If there are form errors then form_success will not be set.

@pmclanahan pmclanahan commented on an outdated diff Feb 10, 2014
bedrock/press/templates/press/speaker-request.html
+ {{ css('press_speaker_request') }}
+{% endblock %}
+
+{% block content %}
+
+<main role="main" id="main-content">
+ <header class="main-header">
+ <h1 class="title-shadow-box">{{_('Speaker request form')}}</h1>
+ </header><!-- /header -->
+
+ <section class="container">
+ {% if not form_success or form.errors %}
+ <h2 class="secondary-title">{{ _('Please complete and submit the form below to request a speaker from Mozilla for your upcoming event.') }}</h2>
+ {% endif %}
+ <div class="main-column">
+ {% if not form_success or form.errors %}
pmclanahan
pmclanahan Feb 10, 2014 Owner

Same as above re: form.errors.

@pmclanahan pmclanahan commented on an outdated diff Feb 10, 2014
bedrock/press/tests.py
+ 'sr_event_name': 'Test Event',
+ 'sr_event_url': 'www.mozilla.org',
+ 'sr_event_date': datetime.date.today() + datetime.timedelta(days=1),
+ 'sr_event_time': '12:00 PM',
+ 'sr_contact_name': 'The Dude',
+ 'sr_contact_email': 'foo@bar.com',
+ }
+
+ def tearDown(self):
+ mail.outbox = []
+
+ def test_view_post_valid_data(self):
+ """
+ A valid POST should 302 redirect.
+ """
+
pmclanahan
pmclanahan Feb 10, 2014 Owner

NIT: extra blank line.

@pmclanahan pmclanahan commented on the diff Feb 10, 2014
bedrock/press/tests.py
+ 'sr_contact_name': 'The Dude',
+ 'sr_contact_email': 'foo@bar.com',
+ }
+
+ def tearDown(self):
+ mail.outbox = []
+
+ def test_view_post_valid_data(self):
+ """
+ A valid POST should 302 redirect.
+ """
+
+ request = self.factory.post(self.url, self.data)
+
+ # make sure CSRF doesn't hold us up
+ request._dont_enforce_csrf_checks = True
pmclanahan
pmclanahan Feb 10, 2014 Owner

I don't think this does anything here since we're not using the full client and no middleware is involved.

jpetto
jpetto Feb 24, 2014 Member

Without disabling the CSRF check, the request status code is 403, so I think we do still need it.

@pmclanahan pmclanahan commented on an outdated diff Feb 10, 2014
bedrock/press/tests.py
+ form = SpeakerRequestForm(self.data)
+
+ # make sure form is valid
+ ok_(form.is_valid())
+
+ def test_form_missing_data(self):
+ """
+ With incorrect data (missing url), form should not be valid and should
+ have url in the errors hash.
+ """
+ self.data.update(sr_event_url='') # remove required url
+
+ form = SpeakerRequestForm(self.data)
+
+ # make sure form is invalid
+ eq_(False, form.is_valid())
pmclanahan
pmclanahan Feb 10, 2014 Owner

NIT: I prefer ok_(not form.is_valid()) for readability and test intent (really just needs to be falsie and not necessarily exactly False.

@pmclanahan pmclanahan commented on an outdated diff Feb 10, 2014
bedrock/press/tests.py
+
+ form = SpeakerRequestForm(self.data, {'sr_attachment': mock_attachment})
+
+ # make sure form is not valid
+ ok_(not form.is_valid())
+
+ # make sure attachment errors are in form
+ self.assertIn('sr_attachment', form.errors)
+
+ @patch('bedrock.press.views.jingo.render_to_string', return_value='jingo rendered')
+ @patch('bedrock.press.views.EmailMessage')
+ def test_email(self, mock_email_message, mock_render_to_string):
+ """
+ Make sure email is sent with expected values.
+ """
+ mock_send = Mock()
pmclanahan
pmclanahan Feb 10, 2014 Owner

You don't need to do all this hoop-jumping to get the send() mock. This should work:

mock_send = mock_email_message.return_value.send
@pmclanahan pmclanahan and 1 other commented on an outdated diff Feb 10, 2014
bedrock/press/urls.py
@@ -0,0 +1,11 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from django.conf.urls import patterns, url
+
+import views
pmclanahan
pmclanahan Feb 10, 2014 Owner

Should be more explicit I think and say import .views

jpetto
jpetto Feb 24, 2014 Member

Had to do from . import views.

Owner

Just a couple more tiny things and we'll be ready to merge. Great work! 🍰

Member
jpetto commented Feb 24, 2014

Since it's been 2 weeks, made last round of fixes in a separate commit. Hopefully we'll be good to merge after a squash. 😄

Owner

r+ !! 👏 squarsh and ship it!

@pmclanahan pmclanahan merged commit ec4bd19 into mozilla:master Feb 24, 2014

1 check passed

default Jenkins build 'bedrock_github' #3297 has succeeded
Details
@jpetto jpetto deleted the jpetto:bug-940555-port-speaker-request-to-bedrock branch Nov 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment