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

Event reminder: Allow to send description for lectures #3157

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

bpedersen2
Copy link
Contributor

Redid the pull request on branch 2.1-dev

As for the plain text problem: Currently the apporaoch is to use striptags in the template.

@ThiefMaster ThiefMaster changed the title Lecturereminders2 Event reminder: Allow to send description for lectures Nov 17, 2017

# revision identifiers, used by Alembic.
revision = '40d4569835b8'
down_revision = None
Copy link
Member

Choose a reason for hiding this comment

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

There are already some alembic revisions in 2.1, so down_revision needs to point to the latest revision before this one (also in the "Revises" line in the comment above)

Copy link
Member

Choose a reason for hiding this comment

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

Probably safest to just delete it and regenerate it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


def upgrade():
op.add_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False,
server_default='false'), schema='events')
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation; default is not affecting the database structure so it can be omitted here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

op.add_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False,
server_default='false'), schema='events')
op.alter_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False),
schema='events')
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation

def upgrade():
op.add_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False,
server_default='false'), schema='events')
op.alter_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False),
Copy link
Member

Choose a reason for hiding this comment

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

the one thing that is actually needed here is missing: server_default=None
nothing changed on nullable so it doesn't need to be specified, and default is never needed in an alembic rev.

see https://github.com/indico/indico/blob/v1.9.11.dev18/indico/migrations/versions/201708301158_09fd62e56097_add_oauth_system_app_type.py#L42-L45 for an example on how to add a new column with a server-side default value during creation (for existing rows)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this actually helps how to do it right. Development.md was a bit cryptic in this respect.

@@ -56,6 +56,9 @@ class ReminderForm(IndicoForm):
message = TextAreaField(_('Note'), description=_('A custom message to include in the email.'))
include_summary = BooleanField(_('Include agenda'),
description=_("Includes a simple text version of the event's agenda in the email."))
include_description = BooleanField(_('Include description'),
description=_("Includes a simple text version of the event's"
" description in the email."))
Copy link
Member

Choose a reason for hiding this comment

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

we usually keep the space in the previous line instead of starting the continued string literal with a space in the new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -96,6 +96,12 @@ class EventReminder(db.Model):
nullable=False,
default=False
)
#: If the notification should include the events description.
Copy link
Member

Choose a reason for hiding this comment

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

event's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


{% filter underline %}Description{% endfilter %}

{{event.description|striptags}}
Copy link
Member

Choose a reason for hiding this comment

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

we use spaces inside {{ ... }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return get_template_module('events/reminders/emails/event_reminder.txt', event=event,
url=event.short_external_url, note=note, with_agenda=with_agenda,
with_description = with_description,
Copy link
Member

Choose a reason for hiding this comment

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

the = in kwargs should not be surrounded with spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


def upgrade():
op.add_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False,
server_default='false'), schema='events')
Copy link
Member

Choose a reason for hiding this comment

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

not properly aligned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

def upgrade():
op.add_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False,
server_default='false'), schema='events')
op.alter_column('reminders', sa.Column('include_description', sa.Boolean(), nullable=False, default=False),
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


{% filter underline %}Description{% endfilter %}

{{event.description|striptags}}
Copy link
Member

Choose a reason for hiding this comment

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

spaces after {{ and before }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ThiefMaster
Copy link
Member

ThiefMaster commented Nov 17, 2017

FYI, the email address you use on your Git commits is not linked to your GitHub account (so the commits show up as anonymous on github); you might want to add your tum email address to your github account or, if you want to use a different email, update the commits and force-push.

@@ -64,8 +67,6 @@ def __init__(self, *args, **kwargs):
self.reply_to_address.choices = (self.event
.get_allowed_sender_emails(extra=self.reply_to_address.object_data)
.items())
if self.event.type_ == EventType.lecture:
Copy link
Member

Choose a reason for hiding this comment

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

I think this should stay now that there's the separate option. The summary is still useless for lectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ThiefMaster ThiefMaster added this to the v2.1 milestone Dec 6, 2017
@@ -53,6 +54,10 @@ def dedent(value):
return indentation_re.sub('', value)


def html2text(value):
return EnsureUnicodeExtension.ensure_unicode(clean(value, tags=[], strip=True))
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't bleach.clean just escape the HTML? Shouldn't we remove any tags instead?

Copy link
Member

Choose a reason for hiding this comment

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

strip=True

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, sorry, didn't scroll enough :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I am thinking about here is, wether this can/should be combined with indico.utils.string# text_to_repr() , which adds mainly space removal and truncation on top.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't. That function has a completely different purpose.

Copy link
Member

Choose a reason for hiding this comment

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

>>> s = 'foo &amp; bar <a href="test">xxx</a> 1<2 <test>'

>>> from lxml.html import html5parser
>>> html5parser.fromstring(s).xpath('string()')
'foo & bar xxx 1<2 '

>>> import bleach
>>> bleach.clean(s, strip=True, tags=[])
u'foo &amp; bar xxx 1&lt;2 '

Maybe using html5parser is better here? We use the description in a plaintext context so having escaped entities etc. is not very nice.

Also, I don't think we need the EnsureUnicodeExtension here; new code already uses unicode. So I would move the new html2text implementation to indico/util/string.py and just do the conversion (assuming the input is already unicode). It'd also be a good opportunity to add a unit test for it (indico/util/string_test.py, check the other testcases in there for examples)

@ThiefMaster ThiefMaster changed the base branch from v2.1-dev to master January 12, 2018 15:01
@bpedersen2
Copy link
Contributor Author

Is there still anything left to change here?


# revision identifiers, used by Alembic.
revision = 'c820455976ba'
down_revision = '1d512a9ebb30'
Copy link
Member

Choose a reason for hiding this comment

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

2af245be72a6

Copy link
Member

Choose a reason for hiding this comment

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

Also rename the file to use the current date in the filename; otherwise it's not in the right order with the revisions added in master since you opened the PR (ie before changing anything, rebase to the latest upstream master)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"""Add include_description to reminders

Revision ID: c820455976ba
Revises: 1d512a9ebb30
Copy link
Member

Choose a reason for hiding this comment

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

2af245be72a6


{% filter underline %}Description{% endfilter %}

{{ event.description| html2text }}
Copy link
Member

Choose a reason for hiding this comment

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

missing space before |

Copy link
Member

Choose a reason for hiding this comment

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

Adding | trim after converting to text would probably be a good idea as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -56,6 +56,9 @@ class ReminderForm(IndicoForm):
message = TextAreaField(_('Note'), description=_('A custom message to include in the email.'))
include_summary = BooleanField(_('Include agenda'),
description=_("Includes a simple text version of the event's agenda in the email."))
include_description = BooleanField(_('Include description'),
description=_("Includes a simple text version of the event's "
Copy link
Member

Choose a reason for hiding this comment

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

I think "a simple text version of " is not needed here; in case of the agenda we have it to make it clear we don't include a very fancy timetable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ThiefMaster
Copy link
Member

After the things I mentioned I think this is ready. I just tested it (already without bleach) and it looked pretty good. Only thing missing is a CHANGES.rst entry; feel free to copy&paste this one (into the improvements section of 2.1):

- Add option to include the event description in reminder emails
  (:issue:`3157`, thanks :user:`bpedersen2`)

@bpedersen2
Copy link
Contributor Author

bumped the migrations to follow upstream

@bpedersen2 bpedersen2 force-pushed the lecturereminders2 branch 3 times, most recently from 461b648 to 1501638 Compare January 29, 2018 06:53
As lectures do not have a time table, the description often
is a good substitute for reminder emails.

Add an option to include the description in the reminder email
(as plain text). The inclusion is not rectricted to lectures.
@ThiefMaster ThiefMaster merged commit bec64fa into indico:master Jan 29, 2018
@ThiefMaster
Copy link
Member

thanks!

@bpedersen2 bpedersen2 deleted the lecturereminders2 branch January 29, 2018 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants