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

Session types #3189

Merged
merged 18 commits into from Jan 25, 2018
Merged

Session types #3189

merged 18 commits into from Jan 25, 2018

Conversation

meluru
Copy link
Contributor

@meluru meluru commented Dec 14, 2017

No description provided.

@meluru meluru changed the base branch from master to v2.1-dev December 14, 2017 15:57

.action-column {
white-space: nowrap;
width: 1px;
Copy link
Member

Choose a reason for hiding this comment

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

1px? why?

Copy link
Member

Choose a reason for hiding this comment

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

to use only as much space as actually needed for its contents

sa.Column('id', sa.Integer(), nullable=False),
sa.Column('event_id', sa.Integer(), nullable=False),
sa.Column('name', sa.String(), nullable=False),
sa.Column('is_poster', sa.Boolean(), nullable=False),
Copy link
Member

Choose a reason for hiding this comment

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

what about server_default since it is not nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we use server_default only when we add a new column to an existing table, so that the old records have some value.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, no need to do that when creating a new table

Copy link
Member

Choose a reason for hiding this comment

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

ahh, sorry for that, didn't notice that it is indeed a new table ;)

@@ -52,6 +54,15 @@
_bp.add_url_rule('/manage/sessions/<int:session_id>/acl', 'acl', RHSessionACL)
_bp.add_url_rule('/manage/sessions/<int:session_id>/acl-message', 'acl_message', RHSessionACLMessage)

# Session types
_bp.add_url_rule('/manage/sessions/types/', 'manage_types', RHManageSessionTypes)
_bp.add_url_rule('/manage/sessions/types/create', 'create_type', RHCreateSessionType,
Copy link
Member

Choose a reason for hiding this comment

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

what maxlinelength are you using in your IDE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

120, but it's true that I didn't need to wrap this one :)

@@ -0,0 +1,18 @@
{% macro types_table_row(session_type) %}
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplicate of what we have for contribution types. Maybe it would be worth it to extract it to a more generic template that could be used in different places?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's really worth the effort or even makes much sense since the poster column is only used here, and there are quite a few references to "session types" in other lines (e.g. confirmation messages), so the amount of reusable code isn't that big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @ThiefMaster At first I thought about making some parts more generic, but I figured it's not really worth complicating.


<div class="manage-session-types">
<div class="toolbar space-after hide-if-locked">
<button id="js-new-session-type" class="i-button icon-plus highlight" data-ajax-dialog
Copy link
Member

Choose a reason for hiding this comment

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

I would js-new-session-type move to the class attribute since this is what we have been doing so far ;)

Copy link
Member

Choose a reason for hiding this comment

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

👍 on that; also move the data-ajax-dialog at the end (we put data attrs with no value last)

backref=db.backref(
'session_types',
cascade='all, delete-orphan',
lazy='dynamic'
Copy link
Member

Choose a reason for hiding this comment

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

Why not just a normal lazy relationship?

super(SessionTypeForm, self).__init__(*args, **kwargs)

def validate_name(self, field):
query = self.event.session_types.filter(db.func.lower(SessionType.name) == field.data.lower())
Copy link
Member

Choose a reason for hiding this comment

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

If you get rid of the lazy=dynamic of the relationship, you need to use this instead:

query = SessionType.query.with_parent(self.event).filter(...)

@@ -0,0 +1,18 @@
{% macro types_table_row(session_type) %}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's really worth the effort or even makes much sense since the poster column is only used here, and there are quite a few references to "session types" in other lines (e.g. confirmation messages), so the amount of reusable code isn't that big.


<div class="manage-session-types">
<div class="toolbar space-after hide-if-locked">
<button id="js-new-session-type" class="i-button icon-plus highlight" data-ajax-dialog
Copy link
Member

Choose a reason for hiding this comment

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

👍 on that; also move the data-ajax-dialog at the end (we put data attrs with no value last)

success: function() {
$row.remove();
dialogModified();
if ($('.manage-session-types table tbody tr').length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

=== (and maybe this whole JS block should be in a js file?)

}
}
});

Copy link
Member

Choose a reason for hiding this comment

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

too many blank lines

@@ -195,3 +197,9 @@ def get_session_timetable_pdf(sess, **kwargs):
return TimeTablePlain(sess.event, session.user, showSessions=[sess.id], showDays=[],
sortingCrit=None, ttPDFFormat=pdf_format, pagesize='A4', fontsize='normal',
firstPageNumber=1, showSpeakerAffiliation=False, **kwargs)


def session_type_row(session_type):
Copy link
Member

Choose a reason for hiding this comment

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

render_session_type_row


.action-column {
white-space: nowrap;
width: 1px;
Copy link
Member

Choose a reason for hiding this comment

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

to use only as much space as actually needed for its contents

@meluru meluru force-pushed the session-types branch 2 times, most recently from 20cf0b1 to 9e042b3 Compare December 19, 2017 15:04
evt.preventDefault();
if (data) {
var $lastRow = $('.manage-contribution-types table tr:last');
if ($lastRow.length) {
$lastRow.after(data.html_row);
} else {
$('.manage-contribution-types').trigger('ajaxDialog:reload');
$manageContributionTypestrigger('ajaxDialog:reload');
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Missing dot! (.trigger)

@@ -50,7 +54,8 @@ def _get_session_list_args(event):
subqueryload('blocks').undefer('contribution_count'))
.order_by(db.func.lower(Session.title))
.all())
return {'sessions': sessions, 'default_colors': get_colors()}
types = [{'id': int(t.id), 'title': t.name} for t in SessionType.query.with_parent(event).all()]
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to cast the id?

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed. And the .all() is not necessary; iterating a query does the job already.



class RHManageSessionTypes(RHManageSessionsBase):
"""Dialog to manage the SessionTypes of an event"""
Copy link
Member

Choose a reason for hiding this comment

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

I'd go for "session types" instead of "SessionTypes" as you do in RHManageSessionTypeBase since this is an explanatory docstring.

data-href="{{ url_for('.delete_type', session_type) }}"
data-confirm="{% trans %}Do you really want to delete this session type?{% endtrans %}"
data-title="{% trans %}Confirm deletion{% endtrans %}"></a>
</td>
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

data-href="{{ url_for('.create_type', event) }}"
data-title="{% trans %}Add new session type{% endtrans %}"
data-ajax-dialog>
{% trans %}New session type{% endtrans %}
Copy link
Member

Choose a reason for hiding this comment

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

{%- trans %}

Same for other occurrences below.


def __init__(self, *args, **kwargs):
event = kwargs.pop('event')
super(SessionForm, self).__init__(*args, **kwargs)
if event.type != 'conference':
del self.is_poster
del self.code
self.type.query = SessionType.query.with_parent(event)
if not self.type.query.count():
Copy link
Member

Choose a reason for hiding this comment

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

has_rows()

if type_id is None:
updates['type_id'] = None
else:
type = SessionType.get(type_id)
Copy link
Member

Choose a reason for hiding this comment

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

type_ to avoid shadowing. also, there's currently no check that the type belongs to the same event!

@@ -50,7 +54,8 @@ def _get_session_list_args(event):
subqueryload('blocks').undefer('contribution_count'))
.order_by(db.func.lower(Session.title))
.all())
return {'sessions': sessions, 'default_colors': get_colors()}
types = [{'id': t.id, 'title': t.name} for t in SessionType.query.with_parent(event)]
Copy link
Member

Choose a reason for hiding this comment

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

why not use the relationship to get the types? event.session_types

sa.PrimaryKeyConstraint('id'),
schema='events'
)
op.create_index(op.f('ix_session_types_event_id'), 'session_types', ['event_id'], unique=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.

use None for the various names (like we do in other migrations) instead of op.f('...')

schema='events'
)
op.create_index(op.f('ix_session_types_event_id'), 'session_types', ['event_id'], unique=False, schema='events')
op.create_foreign_key(op.f('fk_session_types_event_id_events'), 'session_types', 'events', ['event_id'], ['id'],
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -146,16 +151,30 @@ def _process_DELETE(self):
def _process_PATCH(self):
data = request.json
updates = {}
if set(data.viewkeys()) > {'colors'}:
if set(data.viewkeys()) > {'colors', 'type_id'}:
Copy link
Member

Choose a reason for hiding this comment

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

this isn't correct:

>>> {'a', 'c'} > {'a', 'b'}
False

What we actually want here is set(data.viewkeys()) - {'colors', 'type_id'} to fail if there is anything besides the allowed ones.

FWIW, with a single value in the setusing > worked, but with more than one on the right side it doesn't.

def _process(self):
types = SessionType.query.with_parent(self.event).all()
return jsonify_template('events/sessions/management/types_dialog.html', event=self.event,
types=types)
Copy link
Member

Choose a reason for hiding this comment

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

types=self.event.session_types

class RHCreateSessionType(RHManageSessionsBase):
"""Dialog to add a SessionType"""

def _get_response(self, new_type):
Copy link
Member

Choose a reason for hiding this comment

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

no need to split this into a separate method; we only did it in some other places where we wanted to override it in a subclass

"""Dialog to add a SessionType"""

def _get_response(self, new_type):
types = [{'id': t.id, 'title': t.name} for t in SessionType.query.with_parent(self.event)]
Copy link
Member

Choose a reason for hiding this comment

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

self.event.session_types

@@ -131,7 +132,7 @@ <h3>{% trans %}Description{% endtrans %}</h3>
{%- trans %}Contribution list{% endtrans -%}
</a>
{% endif %}
{% if not sess.is_poster %}
{% if not is_poster %}
Copy link
Member

Choose a reason for hiding this comment

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

keep this as it was before and add a property to Session that contains the logic above


$manageContributionTypes.on('ajaxDialog:closed', '.js-edit-contribution-type', function(evt, data) {
evt.preventDefault();
var $row = $(this).closest('tr');
Copy link
Member

Choose a reason for hiding this comment

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

You are using it only if data is not empty. It should be better to move it inside the if.

$(this).closest('tr').replaceWith(data.html_row);

$manageContributionTypes.on('indico:confirmed', '.js-delete-contribution-type', function(evt) {
evt.preventDefault();
var $this = $(this);
var $row = $this.closest('tr');
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 be also somewhere else, namely inside the success callback.

@@ -0,0 +1,61 @@
(function(global) {
Copy link
Member

Choose a reason for hiding this comment

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

This file differs from contributions/types_dialog.js only in one class name. Shouldn't it be one file to make it more generic and not repeat the code?

@ThiefMaster ThiefMaster added this to the v2.1 milestone Jan 10, 2018
@ThiefMaster ThiefMaster changed the base branch from v2.1-dev to master January 12, 2018 14:58
@@ -205,6 +205,10 @@ def conveners(self):
.distinct(SessionBlockPersonLink.person_id)
.all())

@property
def is_poster(self):
return self.type and self.type.is_poster
Copy link
Member

Choose a reason for hiding this comment

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

that'll return None instead of False if there's no type. use self.type.is_poster if self.type else False instead

sa.Column('name', sa.String(), nullable=False),
sa.Column('is_poster', sa.Boolean(), nullable=False),
sa.ForeignKeyConstraint(['event_id'], [u'events.events.id']),
sa.PrimaryKeyConstraint('id', name=op.f('pk_session_types')),
Copy link
Member

Choose a reason for hiding this comment

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

no need to specify the name

sa.Column('event_id', sa.Integer(), nullable=False),
sa.Column('name', sa.String(), nullable=False),
sa.Column('is_poster', sa.Boolean(), nullable=False),
sa.ForeignKeyConstraint(['event_id'], [u'events.events.id']),
Copy link
Member

Choose a reason for hiding this comment

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

no need for the unicode prefix

sa.ForeignKeyConstraint(['event_id'], [u'events.events.id']),
sa.PrimaryKeyConstraint('id', name=op.f('pk_session_types')),
schema='events')
op.create_index(None, 'session_types', ['event_id'], unique=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.

this can go away in favor of index=True on the column definition above

FROM events.sessions
WHERE is_poster = true
GROUP BY event_id;
UPDATE events.sessions
Copy link
Member

Choose a reason for hiding this comment

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

linebreak for readability, or just use two separate op.execute() calls

# Migrate poster sessions to sessions with poster session type
op.execute('''
INSERT INTO events.session_types (event_id, name, is_poster)
SELECT event_id, 'poster session', true
Copy link
Member

Choose a reason for hiding this comment

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

uppercase P in poster?

INSERT INTO events.session_types (event_id, name, is_poster)
SELECT event_id, 'poster session', true
FROM events.sessions
WHERE is_poster = true
Copy link
Member

Choose a reason for hiding this comment

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

WHERE is_poster is enough (just like in python ;))

WHERE is_poster = true
GROUP BY event_id;
UPDATE events.sessions
SET type_id = (SELECT events.session_types.id FROM events.session_types
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify this using UPDATE...FROM (untested)

UPDATE events.sessions s
SET s.type_id = st.id
FROM events.session_types st
WHERE st.event_id = s.event_id AND s.is_poster;

op.execute('''
UPDATE events.sessions
SET is_poster = true
WHERE (SELECT events.session_types.id FROM events.session_types
Copy link
Member

Choose a reason for hiding this comment

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

You can use UPDATE..FROM here as well (again, untested):

UPDATE events.session s
SET s.is_poster = true
FROM event.session_types st
WHERE st.id = s.type_id AND st.is_poster;

@@ -146,16 +151,30 @@ def _process_DELETE(self):
def _process_PATCH(self):
data = request.json
updates = {}
if set(data.viewkeys()) > {'colors'}:
if set(data.viewkeys()) - {'colors', 'type_id'}:
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking here, but wouldn't set(data) give you the same?

@meluru meluru force-pushed the session-types branch 3 times, most recently from 80b1306 to 5c7f095 Compare January 18, 2018 10:55
@meluru meluru changed the title WIP: Session types Session types Jan 22, 2018
CHANGES.rst Outdated
@@ -28,13 +28,17 @@ Improvements
by users submitting an abstract (:issue:`3138`)
- Add support for boolean (yes/no) and freetext questions in abstract
reviewing (:issue:`3175`)
- Add session types to sessions; allow marking session types as poster
so that the sessions with poster session type are displayed differently
in the timetable
Copy link
Member

Choose a reason for hiding this comment

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

poster sessions aren't a new feature, so I would change this a bit, maybe something like this:

- Add support for custom session types (:issue:`3189`)
- Move poster session flag from session settings to session type settings

CHANGES.rst Outdated
the ``can_manage`` method whose ``role`` argument has been renamed to
``permission`` (:issue:`3057`)
- Change poster sessions to sessions with poster session type
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is really worth mentioning here; the idea of the "internal changes" section is to point out things likely to be relevant to developers

CHANGES.rst Outdated

Internal Changes
^^^^^^^^^^^^^^^^

- Rename *Roles* in ACL entries to *Permissions*. This especially affects
- Rename *Roles* in ACL entries to *Permissions*. This especially affects
Copy link
Member

Choose a reason for hiding this comment

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

The double spaces are intentional. In plaintext blobs it's not too uncommon to do this to keep some more spacing between sentences. You can find this in many docstrings or e.g. the GPL license text as well.

th, td {
white-space: normal;

&.name-column {
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 we don't want this anymore - see #3215

@@ -0,0 +1,18 @@
{% macro types_table_row(session_type) %}
<tr class="i-table">
<td class="i-table name-column">{{ session_type.name }}</td>
Copy link
Member

Choose a reason for hiding this comment

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

remove name-column (see my other comment in the css)

<table class="i-table">
<thead>
<tr class="i-table">
<th class="i-table name-column">
Copy link
Member

Choose a reason for hiding this comment

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

remove name-column (see my other comment in the css)

def _get_session_type_updates(self, type_id):
updates = {}
if type_id is None:
updates['type_id'] = None
Copy link
Member

Choose a reason for hiding this comment

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

Try using 'type' here; like this you use the relationship instead of the FK column

if not type_:
raise BadRequest('Invalid type id')
if not self.session.type or type_id != self.session.type.id:
updates['type_id'] = type_id
Copy link
Member

Choose a reason for hiding this comment

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

same here; you already query the type so it's better to assign that to the relationship instead of the ID to the FK (which will be done internally by SA on flush)

def _process(self):
types = SessionType.query.with_parent(self.event).all()
return jsonify_template('events/sessions/management/types_dialog.html', event=self.event,
types=types)
Copy link
Member

Choose a reason for hiding this comment

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

types=self.event.session_types and the query above can go away

data-items="{{ types | tojson | forceescape }}"
data-href="{{ url_for('.session_rest', sess) }}"
data-method="PATCH"
data-selected-item-id="{{ (sess.type.id if sess.type else None) | tojson }}">
Copy link
Member

Choose a reason for hiding this comment

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

lowercase none in jinja

Copy link
Member

Choose a reason for hiding this comment

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

theoretically you should use | forceescape here as well, but since it's always a number or null it doesn't make a difference

@meluru meluru force-pushed the session-types branch 2 times, most recently from 0377946 to 7e505f4 Compare January 24, 2018 14:53
@meluru
Copy link
Contributor Author

meluru commented Jan 24, 2018

Updated

"""add session types table

Revision ID: 9c4418d7a6aa
Revises: 566d5de4e0e5
Copy link
Member

Choose a reason for hiding this comment

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

2af245be72a6