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

Track-session linking #3069

Merged
merged 5 commits into from Nov 1, 2017

Conversation

Projects
None yet
3 participants
@mvidalgarcia
Member

mvidalgarcia commented Sep 15, 2017

Temporary UI to show the default session linked to track:
image


global.setupAbstractJudgment = function setupAbstractJudgment(options) {
options = $.extend({
mapTrackSession: {}

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 15, 2017

Member

trackSessionMap?

@@ -28,8 +32,16 @@
class TrackForm(IndicoForm):
title = StringField(_('Title'), [DataRequired()])
code = StringField(_('Code'))
default_session = QuerySelectField(_('Default session'), default='', allow_blank=True,
get_label=attrgetter('title'),

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 15, 2017

Member

AFAIKget_label='title' does the job too

default_session_id = db.Column(
db.Integer,
db.ForeignKey('events.sessions.id'),
index=True

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 15, 2017

Member

I'd use an explicit nullable=True

@@ -92,6 +97,10 @@ class Track(DescriptionMixin, db.Model):
lazy=True
)
)
default_session = db.relationship(
'Session',
lazy=True

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 15, 2017

Member

missing backref

This comment has been minimized.

@mvidalgarcia

mvidalgarcia Sep 18, 2017

Member

@pferreir mentioned it might not be needed, what do you think?

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 18, 2017

Member

omitting the backref is usually not a good idea - AFAIK some cascades only work when there's a backref

This comment has been minimized.

@mvidalgarcia

mvidalgarcia Sep 18, 2017

Member
default_session = db.relationship(
    'Session',
    lazy=True,
    backref=db.backref(
        'assigned_tracks',
        lazy=True
    )
)

Something like this?

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 18, 2017

Member

Yes, but I'd call it default_for_tracks and you could just use backref='default_for_tracks' when using all the defaults anyway (which is the case here)

<span class="i-label small default-session"
title='{% trans title=session.title %}Indico will preselect the session "{{ title }}"
whenever an abstract is accepted for this track{% endtrans %}'
style="background-color: #{{ session.background_color }};

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 15, 2017

Member

style="{{ session.css }}"

@mvidalgarcia mvidalgarcia force-pushed the mvidalgarcia:track-session-linking branch from a0ee8c5 to a97435b Sep 18, 2017

description = IndicoMarkdownField(_('Description'), editor=True)

def __init__(self, event, *args, **kwargs):

This comment has been minimized.

@ThiefMaster

ThiefMaster Sep 18, 2017

Member

in all other forms requiring an event we pass it as a kwarg and pop it from kwargs before the super call so I'd do the same here

@ThiefMaster ThiefMaster force-pushed the indico:v2.1-dev branch from 292e1b2 to bca95db Sep 21, 2017

@ThiefMaster ThiefMaster force-pushed the mvidalgarcia:track-session-linking branch from a97435b to ea07f76 Sep 21, 2017

@mvidalgarcia mvidalgarcia force-pushed the mvidalgarcia:track-session-linking branch 2 times, most recently from d9ea8ae to 7a10b7c Sep 21, 2017

@pferreir

pferreir approved these changes Sep 25, 2017 edited

removed

@mvidalgarcia

This comment has been minimized.

Member

mvidalgarcia commented Sep 25, 2017

What do you mean exactly? The pre-select logic is in the last commit and I've modified it to trigger the event when loading the page for the first time.

@pferreir

This comment has been minimized.

Member

pferreir commented Sep 25, 2017

Sorry, I started the comment but then forgot to delete it. Of course you've already done it.

@ThiefMaster ThiefMaster force-pushed the indico:v2.1-dev branch from bca95db to ab98259 Oct 12, 2017

@ThiefMaster ThiefMaster added this to the v2.1 milestone Oct 13, 2017

@ThiefMaster ThiefMaster force-pushed the indico:v2.1-dev branch from ab98259 to bd6c466 Oct 27, 2017

@ThiefMaster ThiefMaster force-pushed the mvidalgarcia:track-session-linking branch from 7a10b7c to 5131287 Oct 27, 2017

@ThiefMaster

This comment has been minimized.

Member

ThiefMaster commented Oct 27, 2017

Shouldn't this also work with bulk abstract judgment?

@mvidalgarcia mvidalgarcia force-pushed the mvidalgarcia:track-session-linking branch from 5131287 to 8a013bb Oct 31, 2017

if self.list_generator.static_link_used:
return redirect(self.list_generator.get_list_url())
kwargs = self.list_generator.get_list_kwargs()
kwargs['track_session_map'] = {track.id: track.default_session_id for track in self.event.tracks}

This comment has been minimized.

@mvidalgarcia

mvidalgarcia Oct 31, 2017

Member

Not sure if this is the best way of passing track_session_map to the template, I just didn't want to add it in DisplayAbstractListMixin since it's probably being used by other classes.

This comment has been minimized.

@ThiefMaster

ThiefMaster Oct 31, 2017

Member

Why not override get_list_kwargs in AbstractListGeneratorManagement?

This comment has been minimized.

@ThiefMaster

ThiefMaster Oct 31, 2017

Member

Or hm, it's not really related to the list generator. So maybe adding it there wouldn't be the best choice.

Why not override _render_template and add it to the kwargs in there, and then call super?

This comment has been minimized.

@mvidalgarcia

mvidalgarcia Oct 31, 2017

Member

Good point, updated.

This comment has been minimized.

@mvidalgarcia

mvidalgarcia Oct 31, 2017

Member

Whoops I missed your 2nd comment

This comment has been minimized.

@mvidalgarcia

@mvidalgarcia mvidalgarcia force-pushed the mvidalgarcia:track-session-linking branch from 8a013bb to dbb1d4a Oct 31, 2017

@mvidalgarcia mvidalgarcia force-pushed the mvidalgarcia:track-session-linking branch from dbb1d4a to f66fd42 Oct 31, 2017

<span class="i-label small">
{{ track.code }}
</span>
{% set session = track.default_session %}

This comment has been minimized.

@ThiefMaster

ThiefMaster Nov 1, 2017

Member

that's overwriting the flask session which is used e.g. for session.user. it only doesn't break stuff because we only access the session before this line and not after it.

actually, i don't think there's a need for this assignment at all - you even use track.default_session again in L52..

(i'll fix it)

@ThiefMaster ThiefMaster merged commit 4aa5bc1 into indico:v2.1-dev Nov 1, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@mvidalgarcia mvidalgarcia deleted the mvidalgarcia:track-session-linking branch Jan 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment