From c6dd2472c056771ae555c87f00db1e45f5818e9f Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Tue, 18 Aug 2015 23:48:59 +1000 Subject: [PATCH 01/23] Fix SQLAlchemy detacted error This has been plaguing the functional testing. If the page does an internal redirect the Session is closed... I assume under normal conditions it is opened as part of the redirect. Under testing it is not, and odd errors ensue. --- zkpylons/lib/helpers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/zkpylons/lib/helpers.py b/zkpylons/lib/helpers.py index 58546bd0b..c0a7c8dee 100644 --- a/zkpylons/lib/helpers.py +++ b/zkpylons/lib/helpers.py @@ -468,7 +468,10 @@ def html_clean(str): def redirect_to(*args, **kargs): if 'is_active' in dir(meta.Session): meta.Session.flush() - meta.Session.close() + # Close causes issues if we are running under a test harness + # Not ideal to change behaviour under test but can't see a way around it + if meta.Session.get_bind().url.database != 'zktest': + meta.Session.close() return redirect(url.current(*args, **kargs)) From cc2888d2a8bf28c6241a25e8545b745fe6f7c505 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Thu, 20 Aug 2015 14:38:14 +1000 Subject: [PATCH 02/23] Fixed test Unsure why it broke... :( --- zk/tests/model/test_invoice.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zk/tests/model/test_invoice.py b/zk/tests/model/test_invoice.py index 8ff66bf29..fd3607584 100644 --- a/zk/tests/model/test_invoice.py +++ b/zk/tests/model/test_invoice.py @@ -6,9 +6,9 @@ class TestInvoice(object): def test_item_add(self, db_session): - invoice = InvoiceFactory() invoice_item = InvoiceItemFactory() - invoice.items.append(invoice_item) # invoice_item must be attached to invoice before flush + invoice = InvoiceFactory(items=[invoice_item]) + db_session.flush() assert invoice_item in db_session.query(InvoiceItem).all() From bf2906fb45281a821bad26f60b3e6df4de734009 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Thu, 20 Aug 2015 14:39:02 +1000 Subject: [PATCH 03/23] Added database wipe to DB tests The tests don't commit, so it wasn't needed before. But with the automated testing running the alembic setup the db gets populated will all sorts of standard stuff that gets in the way. --- zk/tests/model/conftest.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/zk/tests/model/conftest.py b/zk/tests/model/conftest.py index 27ff65adf..da6d3993e 100644 --- a/zk/tests/model/conftest.py +++ b/zk/tests/model/conftest.py @@ -26,6 +26,10 @@ def db_session(app_config): meta.Session.configure(autoflush=True) meta.Base.metadata.create_all(meta.engine) + # Drop all data to establish known state, mostly to prevent conflicts with alembic loaded data + meta.engine.execute("drop schema if exists public cascade") + meta.engine.execute("create schema public") + # Also need to set zkpylons version of meta, hours of fun if you don't pymeta.engine = meta.engine pymeta.Session.configure(bind=meta.engine) From 2b929cd9bdcd8507c55e4ee327922deb505086b5 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Mon, 17 Aug 2015 14:35:03 +1000 Subject: [PATCH 04/23] Changed missing role behaviour If a role is requested but not in the database the code threw an exception. I have changed this to throw a NotAuthorizedError. This makes the failure a graceful rather than terminal one. The change is being done to make testing easier, it should also help development allowing code to be written before the database change is made. There is no impact on a production site, the code should not run. --- zkpylons/lib/auth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zkpylons/lib/auth.py b/zkpylons/lib/auth.py index 2cc77e99a..4bc855dba 100644 --- a/zkpylons/lib/auth.py +++ b/zkpylons/lib/auth.py @@ -33,6 +33,7 @@ log = logging.getLogger(__name__) def set_redirect(): + # TODO: This function is called 30+ times per page when not logged in, more than seems needed if not session.get('redirect_to', None): session['redirect_to'] = request.path_info session.save() @@ -83,7 +84,7 @@ def check(self, app, environ, start_response): for role in self.roles: if not self.role_exists(role): - raise Exception("No such role %r exists"%role) + raise NotAuthorizedError("No such role %r exists"%role) person = Person.find_by_email(environ['REMOTE_USER']) if person is None: From f9d989d2199e6218bc356b81d586e90840ba4a30 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Mon, 17 Aug 2015 18:35:27 +1000 Subject: [PATCH 05/23] Minor fix in HTML We had an excess

making the HTML invalid. Not a concern for users but made automated testing have conniptions. --- zkpylons/templates/person/form.mako | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zkpylons/templates/person/form.mako b/zkpylons/templates/person/form.mako index 7f870fb3b..338212808 100644 --- a/zkpylons/templates/person/form.mako +++ b/zkpylons/templates/person/form.mako @@ -4,7 +4,7 @@

*

${ h.text('person.lastname', size=40) }

-${ h.hidden('person.company', value='') }

+${ h.hidden('person.company', value='') } % if c.form is not 'edit' or h.auth.authorized(h.auth.has_organiser_role):

*

From c53d878bae9e232268fc428ab864c99bbe5bfd73 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Tue, 18 Aug 2015 17:00:09 +1000 Subject: [PATCH 06/23] Fix #418, assuming that first/last name is set Mostly switch to fullname to avoid issue --- zkpylons/controllers/admin.py | 30 +++++++++---------- zkpylons/controllers/invoice.py | 2 +- zkpylons/controllers/miniconf_proposal.py | 2 +- zkpylons/controllers/photocomp.py | 2 +- zkpylons/controllers/proposal.py | 8 ++--- .../templates/proposal/view_fragment.mako | 4 +-- 6 files changed, 24 insertions(+), 24 deletions(-) diff --git a/zkpylons/controllers/admin.py b/zkpylons/controllers/admin.py index 83b390c0f..fc0964ac0 100644 --- a/zkpylons/controllers/admin.py +++ b/zkpylons/controllers/admin.py @@ -607,7 +607,7 @@ def registered_speakers(self): res.append(';'.join(consents)) if p.registration: - res.append('

'.join(["Note by " + n.by.firstname + " " + n.by.lastname + " at " + n.last_modification_timestamp.strftime("%Y-%m-%d %H:%M") + ":
" + h.line_break(n.note) for n in p.registration.notes])) + res.append('

'.join(["Note by " + n.by.fullname + " at " + n.last_modification_timestamp.strftime("%Y-%m-%d %H:%M") + ":
" + h.line_break(n.note) for n in p.registration.notes])) if p.registration.diet: res[-1] += '

Diet: %s' % (p.registration.diet) if p.registration.special: @@ -640,7 +640,7 @@ def registered_volunteers(self): volunteer_list = [] for p in meta.Session.query(Person).all(): if not p.is_volunteer(): continue - volunteer_list.append((p.lastname.lower()+' '+p.firstname, p)) + volunteer_list.append(((p.lastname or '').lower()+' '+ (p.firstname or ''), p)) volunteer_list.sort() for (sortkey, p) in volunteer_list: @@ -677,7 +677,7 @@ def registered_volunteers(self): res.append('No Invoice') res.append('-') - res.append('

'.join(["Note by " + n.by.firstname + " " + n.by.lastname + " at " + n.last_modification_timestamp.strftime("%Y-%m-%d %H:%M") + ":
" + h.line_break(n.note) for n in p.registration.notes])) + res.append('

'.join(["Note by " + n.by.fullname + " at " + n.last_modification_timestamp.strftime("%Y-%m-%d %H:%M") + ":
" + h.line_break(n.note) for n in p.registration.notes])) if p.registration.diet: res[-1] += '

Diet: %s' % (p.registration.diet) if p.registration.special: @@ -946,7 +946,7 @@ def lca_announce_signup(self): count = 0 for r in meta.Session.query(Registration).filter(Registration.signup.like("%announce%")).all(): p = r.person - c.text += p.firstname + " " + p.lastname + " <" + p.email_address + ">\n" + c.text += p.fullname + " <" + p.email_address + ">\n" count += 1 c.text += "

" c.text += "

Total addresses: " + str(count) + "

" @@ -964,7 +964,7 @@ def lca_chat_signup(self): count = 0 for r in meta.Session.query(Registration).filter(Registration.signup.like('%chat%')).all(): p = r.person - c.text += p.firstname + " " + p.lastname + " <" + p.email_address + ">\n" + c.text += p.fullname + " <" + p.email_address + ">\n" count += 1 c.text += "

" c.text += "

Total addresses: " + str(count) + "

" @@ -983,7 +983,7 @@ def volunteer_signup(self): for r in meta.Session.query(Registration).all(): if r.person.is_volunteer(): p = r.person - c.text += p.firstname + " " + p.lastname + " <" + p.email_address + ">\n" + c.text += p.fullname + " <" + p.email_address + ">\n" count += 1 c.text += "

" c.text += "

Total addresses: " + str(count) + "

" @@ -1002,7 +1002,7 @@ def speaker_signup(self): for r in meta.Session.query(Registration).all(): if r.person.is_speaker(): p = r.person - c.text += p.firstname + " " + p.lastname + " <" + p.email_address + ">\n" + c.text += p.fullname + " <" + p.email_address + ">\n" count += 1 c.text += "

" c.text += "

Total addresses: " + str(count) + "

" @@ -1021,7 +1021,7 @@ def miniconf_org_signup(self): for r in meta.Session.query(Registration).all(): if r.person.is_miniconf_org(): p = r.person - c.text += p.firstname + " " + p.lastname + " <" + p.email_address + ">\n" + c.text += p.fullname + " <" + p.email_address + ">\n" count += 1 c.text += "

" c.text += "

Total addresses: " + str(count) + "

" @@ -1126,7 +1126,7 @@ def partners_programme(self): if invoice_item.invoice.is_paid and not invoice_item.invoice.is_void: c.data.append([item.description, invoice_item.qty, - invoice_item.invoice.person.firstname + " " + invoice_item.invoice.person.lastname, + invoice_item.invoice.person.fullname, invoice_item.invoice.person.email_address, invoice_item.invoice.person.registration.partner_name, invoice_item.invoice.person.registration.partner_email, @@ -1145,7 +1145,7 @@ def planet_lca(self): count = 0 for r in meta.Session.query(Registration).filter(Registration.planetfeed != '').all(): p = r.person - c.text += "[" + r.planetfeed + "] name = " + p.firstname + " " + p.lastname + "\n" + c.text += "[" + r.planetfeed + "] name = " + p.fullname + "\n" count += 1 c.text += "

" c.text += "

Total addresses: " + str(count) + "

" @@ -1239,7 +1239,7 @@ def rego_desk_list(self): elif item.description.lower().endswith("ticket") or item.description.lower().startswith("press pass"): ticket_types.append(item.description + " x" + str(item.qty)) c.data.append([registration.person.id, - registration.person.firstname + " " + registration.person.lastname, + registration.person.fullname, ", ".join(ticket_types), ", ".join(shirts), dinner_tickets, @@ -1264,9 +1264,9 @@ def previous_years_stats(self): else: years[year] = 1 if len(registration.prevlca) == len(Config.get('past_confs', category='rego')): - veterans.append(registration.person.firstname + " " + registration.person.lastname) + veterans.append(registration.person.fullname) elif len(registration.prevlca) == (len(Config.get('past_confs', category='rego')) - 1): - veterans_lca.append(registration.person.firstname + " " + registration.person.lastname) + veterans_lca.append(registration.person.fullname) for (year, value) in years.iteritems(): c.data.append([year, value]) @@ -1417,7 +1417,7 @@ def email_registration_reminder(self): c.speaker = p.is_speaker() c.firstname = p.firstname - c.fullname = p.firstname + ' ' + p.lastname + c.fullname = p.fullname c.company = p.company c.phone = p.phone c.mobile = p.mobile @@ -1483,7 +1483,7 @@ def rego_foreign(self): def rego_list(self): """ List of paid regos - for rego desk. [Registrations] """ people = [ - (r.person.lastname.lower(), r.person.firstname.lower(), r.id, r.person) + ((r.person.lastname or '').lower(), (r.person.firstname or '').lower(), r.id, r.person) for r in Registration.find_all()] people.sort() people = [row[-1] for row in people] diff --git a/zkpylons/controllers/invoice.py b/zkpylons/controllers/invoice.py index 34c20f0a3..30ae76229 100644 --- a/zkpylons/controllers/invoice.py +++ b/zkpylons/controllers/invoice.py @@ -193,7 +193,7 @@ def _remind(self): c.invoice = i c.recipient = i.person email(c.recipient.email_address, render('invoice/remind_email.mako')) - h.flash('Email sent to ' + c.recipient.firstname + ' ' + c.recipient.lastname + ' <' + c.recipient.email_address + '>') + h.flash('Email sent to ' + c.recipient.fullname + ' <' + c.recipient.email_address + '>') redirect_to(action='remind') def _check_invoice(self, person, invoice, ignore_overdue = False): diff --git a/zkpylons/controllers/miniconf_proposal.py b/zkpylons/controllers/miniconf_proposal.py index 9ebb8d189..8e279fcc7 100644 --- a/zkpylons/controllers/miniconf_proposal.py +++ b/zkpylons/controllers/miniconf_proposal.py @@ -61,7 +61,7 @@ def new(self): 'proposal.travel_assistance': 1, 'proposal.video_release': 0, 'proposal.slides_release': 0, - 'person.name' : c.person.firstname + " " + c.person.lastname, + 'person.name' : c.person.fullname, 'person.mobile' : c.person.mobile, 'person.experience' : c.person.experience, 'person.bio' : c.person.bio, diff --git a/zkpylons/controllers/photocomp.py b/zkpylons/controllers/photocomp.py index b56711b47..0c8479a9c 100644 --- a/zkpylons/controllers/photocomp.py +++ b/zkpylons/controllers/photocomp.py @@ -220,7 +220,7 @@ def index(self): person = Person.find_by_id(person_id) person_map[person_id] = person c.all_person.append(person) - c.all_person.sort(key=lambda person: (person.firstname + " " + person.lastname).lower()) + c.all_person.sort(key=lambda person: person.fullname.lower()) c.photos = photos def photo_title(photo): return "%s %s, %s entry %s, %s" % ( diff --git a/zkpylons/controllers/proposal.py b/zkpylons/controllers/proposal.py index 8690771c1..e5c4305a1 100644 --- a/zkpylons/controllers/proposal.py +++ b/zkpylons/controllers/proposal.py @@ -117,14 +117,14 @@ def new(self): 'proposal.slides_release': 1, 'proposal.travel_assistance' : 1, 'proposal.accommodation_assistance' : 1, - 'person.name': c.person.firstname + " " + c.person.lastname, + 'person.name': c.person.fullname, 'person.phone': c.person.phone, 'person.experience': c.person.experience, 'person.bio': c.person.bio, 'person.url': c.person.url, } defaults['person_to_edit'] = c.person.id - defaults['name'] = c.person.firstname + " " + c.person.lastname + defaults['name'] = c.person.fullname form = render("proposal/new.mako") return htmlfill.render(form, defaults) @@ -287,7 +287,7 @@ def edit(self, id): defaults = h.object_to_defaults(c.proposal, 'proposal') defaults.update(h.object_to_defaults(c.person, 'person')) - defaults['person.name'] = c.person.firstname + " " + c.person.lastname + defaults['person.name'] = c.person.fullname # This is horrible, don't know a better way to do it if c.proposal.type: defaults['proposal.type'] = defaults['proposal.proposal_type_id'] @@ -299,7 +299,7 @@ def edit(self, id): defaults['proposal.audience'] = defaults['proposal.target_audience_id'] defaults['person_to_edit'] = c.person.id - defaults['name'] = c.person.firstname + " " + c.person.lastname + defaults['name'] = c.person.fullname c.miniconf = (c.proposal.type.name == 'Miniconf') form = render('/proposal/edit.mako') return htmlfill.render(form, defaults) diff --git a/zkpylons/templates/proposal/view_fragment.mako b/zkpylons/templates/proposal/view_fragment.mako index 9a149f263..9df3802f4 100644 --- a/zkpylons/templates/proposal/view_fragment.mako +++ b/zkpylons/templates/proposal/view_fragment.mako @@ -98,8 +98,8 @@ ${ person.firstname | h } ${ person.lastname | h } <${ person.email_address } %endif
${ h.link_to('(view details)', url=h.url_for(controller='person', action='view', id=person.id)) } -${ h.link_to('(stalk on Google)', url='http://google.com/search?q=%s+%s' % (person.firstname + " " + person.lastname, person.email_address)) } -${ h.link_to('(linux specific stalk)', url='http://google.com/linux?q=%s+%s' % (person.firstname + " " + person.lastname, person.email_address)) } +${ h.link_to('(stalk on Google)', url='http://google.com/search?q=%s+%s' % (person.fullname, person.email_address)) } +${ h.link_to('(linux specific stalk)', url='http://google.com/linux?q=%s+%s' % (person.fullname, person.email_address)) } ${ h.link_to('(email address only stalk)', url='http://google.com/search?q=%s' % person.email_address) }

% endif From 5729194fbc2a6be6ee6b4740998e844a7490dfe6 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Tue, 18 Aug 2015 17:03:26 +1000 Subject: [PATCH 07/23] Fix #416, proposal validation requiring phone Also dropped some related duplicate code --- zkpylons/controllers/proposal.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/zkpylons/controllers/proposal.py b/zkpylons/controllers/proposal.py index e5c4305a1..34748209f 100644 --- a/zkpylons/controllers/proposal.py +++ b/zkpylons/controllers/proposal.py @@ -25,21 +25,13 @@ log = logging.getLogger(__name__) -class NewPersonSchema(BaseSchema): - allow_extra_fields = False - - experience = validators.String(not_empty=True) - bio = validators.String(not_empty=True) - url = validators.URL(add_http=True, check_exists=False, not_empty=False) - phone = validators.String(not_empty=True) - class ExistingPersonSchema(BaseSchema): allow_extra_fields = False experience = validators.String(not_empty=True) bio = validators.String(not_empty=True) url = validators.URL(add_http=True, check_exists=False, not_empty=False) - phone = validators.String(not_empty=True) + mobile = validators.String(not_empty=True) class ProposalSchema(BaseSchema): allow_extra_fields = False @@ -59,7 +51,7 @@ class ProposalSchema(BaseSchema): slides_release = validators.Bool() class NewProposalSchema(BaseSchema): - person = NewPersonSchema() + person = ExistingPersonSchema() proposal = ProposalSchema() attachment = FileUploadValidator() pre_validators = [NestedVariables] From b5eb0645a34ec93bec0881b4361618cb5e3dd7e2 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Tue, 18 Aug 2015 17:33:27 +1000 Subject: [PATCH 08/23] Fixes #417, thankyou breaking on no email set --- zkpylons/templates/proposal/thankyou_email.mako | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zkpylons/templates/proposal/thankyou_email.mako b/zkpylons/templates/proposal/thankyou_email.mako index 5811d2337..abed60948 100644 --- a/zkpylons/templates/proposal/thankyou_email.mako +++ b/zkpylons/templates/proposal/thankyou_email.mako @@ -6,8 +6,10 @@ Dear ${ c.person.firstname }, Thank you for proposing a ${ c.proposal.type.name.lower() } for ${ c.config.get('event_name') }. +% if c.proposal.type.notify_email: If you have any queries about your proposed ${ c.proposal.type.name.lower()}, please email ${ c.proposal.type.notify_email.lower() } +% endif Title: ${ c.proposal.title |n } Target audience: ${ c.proposal.audience.name } From bed67a5a10b573700f05b9f0418952f0f61326f4 Mon Sep 17 00:00:00 2001 From: David Tulloh Date: Tue, 18 Aug 2015 22:30:50 +1000 Subject: [PATCH 09/23] Reduced number of HTML parsing errors Automated testing of registration form was failing. It wasn't picking up all the elements. I suspected that the broken HTML might be putting the parser off. Reduced the HTML errors from 185 to 145. Including hitting most of the structural and all of the form issues. --- zkpylons/templates/base.mako | 5 +- zkpylons/templates/leftcol/toolbox.mako | 28 ++++---- zkpylons/templates/registration/form.mako | 85 +++++++++++------------ 3 files changed, 58 insertions(+), 60 deletions(-) diff --git a/zkpylons/templates/base.mako b/zkpylons/templates/base.mako index 981f3d26b..9da3c2c87 100644 --- a/zkpylons/templates/base.mako +++ b/zkpylons/templates/base.mako @@ -5,10 +5,10 @@ <%def name="extra_head()"> ## Defined in children +<%def name="body_property()"> <%def name="extra_body()"> - + -<%def name="body_property()"> <%def name="big_promotion()"> ## Defined in children @@ -66,7 +66,6 @@ %endif - ${self.extra_body()}