Paper assignment page #2756

mvidalgarcia · 2016-12-09T13:33:48Z

WIP:

Filter by state
Add optional columns: Contribution type, session
Filter by Track, Contribution type, session
~~Extra filters: Material submitted~~
Bulk change state
Update headers in new files by running python bin/maintenance/update_header.py 2017

nop33 · 2016-12-09T15:11:02Z

indico/modules/events/papers/blueprint.py

@@ -34,6 +34,7 @@
                 methods=('GET', 'POST'))
 _bp.add_url_rule('/manage/papers/enable/<any(content,layout):reviewing_type>', 'switch',
                 management.RHSwitchReviewingType, methods=('PUT', 'DELETE'))
+_bp.add_url_rule('/manage/papers/assign', 'assignment', management.RHPapersAssignment)


I'd use RHPaperAssignmentList and /manage/papers/assignment-list/

nop33 · 2016-12-09T15:15:32Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                            {% if contrib.paper_revisions %}
+                                {{ contrib.paper_revisions.0.state.title }}
+                            {% else %}
+                                {%- trans %}Paper not yet submitted{% endtrans -%}


In some places you use - to remove the whitespace and in others you don't. I'd suggest to stick to one of the two conventions. The best IMO is to remove the whitespace because it creates an ugly HTML structure.

nop33 · 2016-12-09T15:16:49Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                    {% for contrib in contribs %}
+                        {% set last_revision_state %}
+                            {% if contrib.paper_revisions %}
+                                {{ contrib.paper_revisions.0.state.title }}


Does this even work? ❗

Why not {{ contrib.paper_revisions[0].state.title }}?

I think we should have a property in the contribution model to retrieve the latest revision...

yes, please do not use .0. in jinja but [0] instead. And yes, we should have a property/relationship that takes care of it. Since the relationship is slighly tricky - but albeit very useful so we can joinedload it e.g. for the listing - I would first go for a simple property and then change it to a relationship later.

@ThiefMaster, muy importante.

nop33 · 2016-12-09T15:21:59Z

indico/modules/events/papers/templates/management/assignment.html

+{% block content %}
+    <div class="toolbars space-after">
+        <div class="toolbar">
+            <div class="group">


As explained in our toolbars wiki page, single buttons in a toolbar don't need to be wrapped by the .group element anymore. Apply this to the rest of your templates as well! We currently have some (quite many actually) templates in Indico that still use the .group element even if it includes 1 button, but we'll remove them eventually. We just gotta make sure that any new code doesn't increase the amount of code we need to change!

nop33 · 2016-12-09T15:23:10Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                            data-title="{{ contrib.title }}">
+                            <td class="i-table id-column">
+                                <span class="vertical-aligner">
+                                    <input type="checkbox" class="select-row" name="contribution_id"


Apart from the class and id attributes, i'd put the rest in a new line each, similar to the way you did it in lines 44-46. Same applies for the data-searchable and data-text below and in general anywhere where the list of attributes needs to wrap.

Not a strong opinion but I would definitely not put type on a new line since it's the most relevant attribute on an <input> tag.

nop33 · 2016-12-09T15:38:19Z

indico/modules/events/papers/templates/management/assignment.html

+            <div class="group">
+                <a href="#" class="i-button icon-checkmark js-enable-if-checked disabled"
+                   data-href="#">
+                    {%- trans %}Accept{% endtrans -%}


I'd group the accept, reject and to be corrected.

I'd move them into a dropdown

I thought about it but wasn't sure about the button label: Mark as, Change state? Suggestions are welcome.

Just wondering if sending papers back for corrections make sense... Shouldn't we enforce leaving some feedback for the submitter?

The comment field should become mandatory in that case.

nop33 · 2016-12-09T15:39:49Z

indico/modules/events/papers/templates/management/assignment.html

+                </a>
+            </div>
+            <div class="group">
+                <a href="#" class="i-button icon-quill js-enable-if-checked disabled"


Not sure about the quill icon. Maybe edit would be better? We use the quill in a complete different context on Indico...

icon-undo maybe? 😕

nop33 · 2016-12-09T15:43:53Z

indico/modules/events/papers/templates/management/assignment.html

+                <a class="i-button icon-checkbox-checked arrow js-dropdown" data-toggle="dropdown"></a>
+                <ul class="dropdown">
+                    <li>
+                        <a href="#" data-select-all="#assignment-list input:checkbox">{% trans %}All{% endtrans %}</a>


Selecting a row doesn't highlight it. Make sure the handleRowSelection method is executed (is should be executed by calling setupListGenerator as we do in the setupContributionList function of the contribution list (contributions/templates/management/contributions.html)

nop33 · 2016-12-09T15:45:16Z

indico/modules/events/papers/templates/management/assignment.html

+                <ul class="dropdown">
+                    <li><a href="#">{% trans %}Judge{% endtrans %}</a></li>
+                    <li><a href="#">{% trans %}Content reviewer{% endtrans %}</a></li>
+                    <li><a href="#">{% trans %}Layout reviewer{% endtrans %}</a></li>


Content and layout reviewer options should be available only if the setting is enabled.

nop33 · 2016-12-09T15:47:45Z

indico/modules/events/papers/templates/management/assignment.html

+        </div>
+        <div class="toolbar">
+            <div class="group" id="filter-statistics">
+                {{ render_displayed_entries_fragment(contribs|length, total_entries) }}


Where did the denominator go? 😄

Forgot to mention that is WIP, my bad. I'm currently working on the list generator that will solve it.

It just tends to infinity.

nop33 · 2016-12-09T15:48:16Z

indico/modules/events/papers/templates/management/assignment.html

+            </div>
+            <div class="group">
+                <span class="i-button label icon-search"></span>
+                <input type="text" id="search-input" placeholder="{% trans %}Enter #id or search string{% endtrans %}">


The live search is not functional.

nop33 · 2016-12-09T15:49:52Z

indico/modules/events/papers/templates/management/assignment.html

+            </div>
+            <div class="group">
+                <a class="i-button arrow js-dropdown" data-toggle="dropdown">
+                    {%- trans %}Assign{% endtrans -%}


The Assign and Unassign buttons should be disabled unless there are checked rows. Use:
js-enable-if-checked disabled

ThiefMaster · 2016-12-09T20:07:17Z

indico/htdocs/js/indico/modules/papers.js

+            placeholder: '#filter-placeholder'
+        };
+
+        $('.list-section [data-toggle=dropdown]').closest('.group').dropdown();


js-dropdown doesn't work?

ThiefMaster · 2016-12-09T20:07:45Z

indico/modules/events/papers/controllers/management.py

+    """Assign contrubutions to reviewers and judges"""
+
+    def _process(self):
+        contribs = sorted(self.event_new.contributions, key=lambda x: x.friendly_id)


key=attrgetter('friendly_id')

Also, you could do that in the Jinja template. event.contributions|sort(attribute='friendly_id')

ThiefMaster · 2016-12-09T20:09:29Z

indico/modules/events/papers/templates/management/assignment.html

+
+{% block description %}
+    {%- trans -%}
+        Assign <strong>judges</strong>, <strong>content reviewers</strong> and <strong>layout reviewers</strong>


judges and reviewers to avoid having to use conditionals depending on what's enabled (for such conditionals you'd need to repeat the whole sentence since you cannot have them inside trans blocks)

ThiefMaster · 2016-12-09T20:09:50Z

indico/modules/events/papers/templates/management/assignment.html

+            </div>
+            <div class="group">
+                <button class="i-button icon-settings js-dialog-action js-customize-list highlight"
+                        data-href="#"


I guess this is WIP?

Yes, forgot to mention in the PR title. I just worked on the table basic structure so far.

I would leave {# TODO: ... #} commets not to forget. It is ok to commit those into devel in early stages of a new release when the blueprint/page is still missing.

ThiefMaster · 2016-12-09T20:10:05Z

indico/modules/events/papers/templates/management/assignment.html

+            <div class="group">
+                <button class="i-button icon-settings js-dialog-action js-customize-list highlight"
+                        data-href="#"
+                        data-title="{% trans %}Paper assignment list configuration{% endtrans %}"


Just "Customize list" or something like that? The title doesn't need to be super verbose

ThiefMaster · 2016-12-09T20:10:20Z

indico/modules/events/papers/templates/management/assignment.html

+            <div class="group">
+                <a href="#" class="i-button icon-checkmark js-enable-if-checked disabled"
+                   data-href="#">
+                    {%- trans %}Accept{% endtrans -%}


I'd move them into a dropdown

OmeGak · 2016-12-12T10:35:35Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+            </table>
+        </form>
+    {%- else %}
+        {%- call message_box('info') %}


I would move this to be the first block. This helps see more clearly the implications of the if block.

{% if not contribs %} {%- call message_box('info') %} ... {% else %} <form method="POST"> ... {% endif %}

OmeGak · 2016-12-12T10:39:33Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                    {% for contrib in contribs %}
+                        {% set last_revision_state %}
+                            {% if contrib.paper_revisions %}
+                                {{ contrib.paper_revisions.0.state.title }}


@ThiefMaster, muy importante.

OmeGak · 2016-12-12T10:44:48Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                </thead>
+                <tbody>
+                    {% for contrib in contribs %}
+                        {% set last_revision_state %}


Not sure if we want to start using set blocks with logic inside... Opinions?

Another way to write this would be:

{% if contrib.paper_revisions %} {% set last_revision_state = contrib.paper_revisions.0.state.title %} {% else %} {% set last_revision_state %} {%- trans %}Paper not yet submitted{% endtrans -%} {% endset %} {% endif %}

I'd avoid set blocks except where useful. If there's lots of logic inside it might be better suited in a macro.

OmeGak · 2016-12-12T10:46:10Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                            data-title="{{ contrib.title }}">
+                            <td class="i-table id-column">
+                                <span class="vertical-aligner">
+                                    <input type="checkbox" class="select-row" name="contribution_id"


Not a strong opinion but I would definitely not put type on a new line since it's the most relevant attribute on an <input> tag.

OmeGak · 2016-12-12T10:49:10Z

indico/modules/events/papers/templates/management/assignment.html

+{% from 'events/management/_lists.html' import render_displayed_entries_fragment %}
+
+{% block title %}
+    {% trans %}Paper Assignment{% endtrans %}


The title should be the same in all paper-related pages. In this case, Call for Papers I believe.
This we can move to the subtitle block.

OmeGak · 2016-12-12T10:51:48Z

indico/modules/events/papers/templates/management/assignment.html

+            </div>
+            <div class="group">
+                <button class="i-button icon-settings js-dialog-action js-customize-list highlight"
+                        data-href="#"


I would leave {# TODO: ... #} commets not to forget. It is ok to commit those into devel in early stages of a new release when the blueprint/page is still missing.

OmeGak · 2016-12-12T10:58:25Z

indico/modules/events/papers/templates/management/assignment.html

+                </a>
+            </div>
+            <div class="group">
+                <a href="#" class="i-button icon-quill js-enable-if-checked disabled"


icon-undo maybe? 😕

OmeGak · 2016-12-12T10:58:52Z

indico/modules/events/papers/templates/management/assignment.html

+        </div>
+        <div class="toolbar">
+            <div class="group" id="filter-statistics">
+                {{ render_displayed_entries_fragment(contribs|length, total_entries) }}


It just tends to infinity.

OmeGak · 2016-12-12T11:00:14Z

indico/modules/events/papers/templates/management/assignment.html

+            <div class="group">
+                <a href="#" class="i-button icon-checkmark js-enable-if-checked disabled"
+                   data-href="#">
+                    {%- trans %}Accept{% endtrans -%}


Just wondering if sending papers back for corrections make sense... Shouldn't we enforce leaving some feedback for the submitter?

nop33 · 2016-12-16T12:41:26Z

indico/modules/events/papers/blueprint.py

@@ -36,6 +36,9 @@
                 methods=('GET', 'POST'))
 _bp.add_url_rule('/manage/papers/enable/<any(content,layout):reviewing_type>', 'switch',
                 management.RHSwitchReviewingType, methods=('PUT', 'DELETE'))
+_bp.add_url_rule('/manage/papers/assignment-list', 'assignment', management.RHPapersAssignmentList)
+_bp.add_url_rule('/manage/papers/assign/customize', 'customize_assignment_list', management.RHAssignmentListCustomize,


I would use /manage/papers/assignment-list/customize here and in the line above /manage/papers/assignment-list/ (slash at the end)

nop33 · 2016-12-16T12:42:35Z

indico/modules/events/papers/lists.py

+
+from collections import OrderedDict
+from operator import attrgetter
+from flask import request


flask < operator

nop33 · 2016-12-16T12:42:56Z

indico/modules/events/papers/lists.py

+from indico.modules.events.papers.models.revisions import PaperRevisionState, PaperRevision
+from indico.modules.events.util import ListGeneratorBase
+from indico.web.flask.templating import get_template_module
+from indico.util.i18n import _


nop33 · 2016-12-16T12:43:49Z

indico/modules/events/papers/lists.py

+        super(PaperAssignmentListGenerator, self).__init__(event)
+        self.default_list_config = {
+            'items': ('state', ),
+            'filters': {'items': {}}}


self.default_list_config = { 'items': ('state', ), 'filters': {'items': {}} }

nop33 · 2016-12-16T12:45:35Z

indico/modules/events/papers/lists.py

+            'items': ('state', ),
+            'filters': {'items': {}}}
+
+        state_not_submitted = {None: 'Not yet submitted'}


Add translations everywhere: _('Not yet submitted')

nop33 · 2016-12-16T12:48:42Z

indico/modules/events/papers/lists.py

+        """Render the contribution list template components.
+
+        :return: dict containing the list's entries, the fragment of
+                 displayed entries and whether the contrib passed is displayed


"...whether the contribution...", it's docstring, no need for abbreviations!

btw, I'd change it also in the contribution list files as well ;)

nop33 · 2016-12-16T12:52:47Z

indico/modules/events/papers/templates/management/assignment.html

+{% block description %}
+    {%- trans -%}
+        Assign <strong>judges</strong> and <strong>reviewers</strong> to contributions in order to allow assessing
+        its paper revisions.


its or theirs? I am not sure which one fits the best.

None of them 😄, I think their fits better

nop33 · 2016-12-16T12:53:26Z

indico/modules/events/papers/templates/management/assignment.html

+                    data-dialog-classes="list-filter-dialog"
+                    data-update='{"html": "#assignment-list", "filter_statistics": "#filter-statistics"}'
+                    data-ajax-dialog>
+                {% trans %}Customize list{% endtrans %}


{%- trans %}Customize list{% endtrans -%}

nop33 · 2016-12-16T12:55:01Z

indico/modules/events/papers/templates/management/assignment_list_filter.html

+    <div class="section">
+        <div class="icon icon-wrench"></div>
+        <div class="text">
+            <div class="label">Customize paper asssignment list</div>


{% trans %}

ThiefMaster · 2016-12-16T13:44:29Z

indico/modules/events/papers/lists.py

+from __future__ import unicode_literals
+
+from collections import OrderedDict
+from flask import request


flask and sqlalchemy are not stdlib

(he means they should be in a separate block)

ThiefMaster · 2016-12-16T13:46:23Z

indico/modules/events/contributions/models/contributions.py

+            uselist=False,
+            lazy=True,
+            viewonly=True,
+            primaryjoin=((PaperRevision.contribution_id == cls.id) &


I'd use db.and_() instead of & - more readable IMO when wrapped

ThiefMaster · 2016-12-16T13:46:59Z

indico/modules/events/contributions/models/contributions.py

@@ -332,6 +332,21 @@ def __table_args__(cls):
        )
    )

+    @declared_attr
+    def paper_last_revision(cls):


latest_paper_revision or current_paper_revision?

As discussed on IRC, paper_last_revision has the upside of namespacing things, which is quite useful to people who (like me) use auto-complete on indico shell.

ThiefMaster · 2016-12-16T13:49:37Z

indico/modules/events/papers/lists.py

+                       'session': Contribution.session_id,
+                       'type': Contribution.type_id}
+        for key, column in filter_cols.iteritems():
+            ids = set(filters['items'].get(key, ()))


the , () is not needed since you check for falsiness in the next line anyway, so the default None is fine

set(None) -> 'NoneType' object is not iterable

ah, didn't spot the set() around it

ThiefMaster · 2016-12-16T13:50:30Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+{% from 'message_box.html' import message_box %}
+
+{% macro render_paper_assignment_list(event, total_entries, contribs, static_columns) %}
+    {% set tz = event.display_tzinfo %}


unused. also, management wouldn't use display_tzinfo but just tzinfo

ThiefMaster · 2016-12-16T13:50:47Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                                <span class="vertical-aligner">
+                                    <input type="checkbox" class="select-row"
+                                           name="contribution_id"
+                                           value="{{ contrib.id }}">


fits in the previous line

I know, I've changed it because of #2756 (comment)

ThiefMaster · 2016-12-16T13:52:03Z

indico/modules/events/papers/templates/management/_paper_assignment_list.html

+                                </span>
+                            </td>
+                            <td class="i-table person-row-cell"
+                                data-searchable="{{ contrib.paper_judges|map(attribute='name')|join(', ')|lower }}">


|join(', ', attribute='name') - no need for the separate |map()

http://jinja.pocoo.org/docs/dev/templates/#join

ThiefMaster · 2016-12-16T13:53:28Z

indico/modules/events/papers/templates/management/assignment_list_filter.html

+                        <li>
+                            <input type="checkbox" name="field_{{ item_id }}" value="{{ value }}"
+                                   id="field_{{ item_id }}_option_{{ value }}"
+                                   {% if value|string in filters[item_id]|string %}


{{ 'checked' if value|string in filters[item_id] }}?

Also, filters[item_id]|string looks very wrong to me. Are you by any chance converting a list/set to a string and then check whether another string is in it?

The problem is when the value is None (in cases like "Not yet submitted", "No track", etc.), then the check is if "None" in [None, ...] which is false and must be true.

Sounds like we should find a better solution then.

{% set value = none if value is none else value|string %}

How about that?

Yeah, that works! 👍

ThiefMaster · 2016-12-16T13:53:51Z

indico/modules/events/papers/templates/management/assignment_list_filter.html

+        </div>
+    </div>
+</div>
+<div class="success-message-box js-clear-filters-message" style="display: none">


pferreir · 2016-12-16T15:46:40Z

indico/modules/events/papers/lists.py

+    def _build_query(self):
+        return (Contribution.query.with_parent(self.event)
+                .order_by(Contribution.friendly_id)
+                .options(subqueryload('paper_last_revision')))


It almost looks easy now.

pferreir · 2016-12-16T15:52:59Z

indico/modules/events/contributions/models/contributions.py

+                         (PaperRevision.submitted_dt == (db.select([db.func.max(PaperRevision.submitted_dt)])
+                                                         .where(PaperRevision.contribution_id == cls.id)
+                                                         .correlate_except(PaperRevision)
+                                                         .as_scalar()))))


Indentation seems wrong.

ThiefMaster · 2016-12-16T16:12:50Z

indico/modules/events/contributions/models/contributions.py

+            lazy=True,
+            viewonly=True,
+            primaryjoin=(db.and_((PaperRevision.contribution_id == cls.id),
+                                 (PaperRevision.submitted_dt == (db.select([db.func.max(PaperRevision.submitted_dt)])


I would assign the subquery to a subquery variable and use PaperRevision.submitted_dt == subquery.as_scalar() here (or even move the as_scalar() to the subquery = ... too)

ThiefMaster · 2016-12-16T16:13:15Z

indico/modules/events/contributions/models/contributions.py

+            uselist=False,
+            lazy=True,
+            viewonly=True,
+            primaryjoin=(db.and_((PaperRevision.contribution_id == cls.id),


no need for the innermost () now; they were only needed because of the precedence of &

nop33 · 2016-12-18T12:06:31Z

indico/modules/events/papers/controllers/management.py

@@ -22,6 +22,7 @@
 from indico.modules.events.papers.forms import PaperTeamsForm, make_competences_form
 from indico.modules.events.papers.operations import (set_reviewing_state, update_team_members, create_competences,
                                                     update_competences)
+from indico.modules.events.papers.lists import PaperAssignmentListGenerator


lists < operations

nop33 · 2016-12-18T12:06:36Z

indico/modules/events/contributions/lists.py

@@ -122,7 +122,7 @@ def render_list(self, contrib=None):
        :param contrib: Used in RHs responsible for CRUD operations on a
                        contribution.
        :return: dict containing the list's entries, the fragment of
-                 displayed entries and whether the contrib passed is displayed
+                 displayed entries and whether the contribution passed is displayed


Docstrings should wrap after 80 characters and python code after 120. Make sure you setup your editor to indicate that 😉

😞 http://stackoverflow.com/questions/24125957/multiple-right-margins-in-pycharm
I'll take it into account tho

nop33 · 2016-12-18T12:07:34Z

indico/modules/events/papers/lists.py

+    def __init__(self, event):
+        super(PaperAssignmentListGenerator, self).__init__(event)
+        self.default_list_config = {
+            'items': ('state', ),


extra space after 'state',

ThiefMaster · 2016-12-21T11:39:43Z

indico/modules/events/papers/blueprint.py

@@ -39,6 +39,8 @@
 _bp.add_url_rule('/manage/papers/assignment-list/', 'assignment', management.RHPapersAssignmentList)
 _bp.add_url_rule('/manage/papers/assignment-list/customize', 'customize_assignment_list',
                 management.RHAssignmentListCustomize, methods=('GET', 'POST'))
+_bp.add_url_rule('/manage/papers/assignment-list/change-state', 'manage_papers_state',


Don't we call this "judging" in the abstracts module (RH name, URL, endpoint name, etc)? In that case I'd use similar naming here for consistency.

ThiefMaster · 2016-12-21T11:41:03Z

indico/modules/events/papers/controllers/management.py

+
+    def _process(self):
+        form = BulkPaperJudgmentForm(event=self.event_new, judgment=request.form.get('judgment'),
+                                     contribution_id=[a.id for a in self.contributions])


Copied from abstracts I guess? ;)

a doesn't fit here much, I'd use c or the generic x (but usually I go for c for contributions)

ThiefMaster · 2016-12-21T11:41:35Z

indico/modules/events/papers/operations.py

@@ -57,3 +61,21 @@ def update_competences(user_competences, competences):
    event.log(EventLogRealm.management, EventLogKind.positive, 'Papers',
              "Updated competences for user {}".format(user_competences.user.full_name), session.user,
              data={'Competences': ', '.join(competences)})
+
+
+def paper_change_state(contribution, contrib_data, judgment, judge, send_notifications=False):


judge_paper?

Just clarifying, the rationale is to use verbs for functions/methods as much as possible.

ThiefMaster · 2016-12-21T11:42:03Z

indico/modules/events/papers/controllers/management.py

+            num_judged_contribs = len(submitted_contributions)
+            num_prejudged_contribs = len(self.contributions) - num_judged_contribs
+            if num_judged_contribs:
+                flash(ngettext("One paper contribution has been judged.",


just "paper" or "contribution's paper" - "paper contribution" sounds rather weird.

ThiefMaster · 2016-12-21T11:45:06Z

indico/modules/events/papers/forms.py

+        if kwargs['judgment']:
+            self._remove_unused_fields(kwargs['judgment'])
+
+    def _remove_unused_fields(self, judgment):


This can go away if we don't have any fields that are specific to certain judgment actions.

ThiefMaster · 2016-12-21T11:45:48Z

indico/modules/events/papers/operations.py

+
+
+def paper_change_state(contribution, contrib_data, judgment, judge, send_notifications=False):
+    log_data = {'New state': orig_string(judgment.title)}


I'd move that down so all the logging stuff is at the end

ThiefMaster · 2016-12-21T11:48:36Z

indico/modules/events/papers/operations.py

+        contribution.paper_last_revision.state = PaperRevisionState.rejected
+    elif judgment == PaperAction.to_be_corrected:
+        contribution.paper_last_revision.state = PaperRevisionState.to_be_corrected
+    PaperReviewComment(paper_revision=contribution.paper_last_revision, user=judge,


So we do want to save the judgment comment as a normal comment in the end and not in a separate field on the revision?

Anyway, while this works (due to being added to the SA session since you link it to he prevision) I think it's slighly ugly to create an instance without using it anywhere. I'd rather do it like this:

if contrib_data['judgment_comment']: comment = PaperReviewComment(user=judge, text=contrib_data['judgment_comment']) contribution.paper_last_revision.comments.append(comment)

I also thought we settled for adding the judgment comment as a field in the revision. Not that I disagree with this approach, but just wondering if I missed something.

ThiefMaster · 2016-12-21T11:49:08Z

indico/modules/events/papers/operations.py

+    log_data['Sent notifications'] = send_notifications
+    if send_notifications:
+        pass  # TODO: manage notifications
+    logger.info('Paper contribution %s changed state by %s', contribution.title, judge)


%r for the second one. I'd also include the new state.

like before, "paper contribution" sounds really strange.

OmeGak · 2017-01-09T15:03:46Z

indico/modules/events/papers/operations.py

@@ -57,3 +61,21 @@ def update_competences(user_competences, competences):
    event.log(EventLogRealm.management, EventLogKind.positive, 'Papers',
              "Updated competences for user {}".format(user_competences.user.full_name), session.user,
              data={'Competences': ', '.join(competences)})
+
+
+def paper_change_state(contribution, contrib_data, judgment, judge, send_notifications=False):


Just clarifying, the rationale is to use verbs for functions/methods as much as possible.

OmeGak · 2017-01-09T15:07:32Z

indico/modules/events/papers/controllers/management.py

+            judgment_data, contrib_data = form.split_data
+            # TODO: For now, I'm considering that any submitted paper contribution can change
+            # its state (even rejected ones)
+            submitted_contributions = [c for c in self.contributions if c.paper_last_revision]


This is actually how many papers have been submitted. Why not num_papers?

OmeGak · 2017-01-09T15:08:32Z

indico/modules/events/papers/controllers/management.py

+            submitted_contributions = [c for c in self.contributions if c.paper_last_revision]
+            for contrib in submitted_contributions:
+                paper_change_state(contrib, contrib_data, judge=session.user, **judgment_data)
+            num_judged_contribs = len(submitted_contributions)


It's the papers that are judged, not the contributions. I would name these num_judged_papers.

OmeGak · 2017-01-09T15:10:13Z

indico/modules/events/papers/controllers/management.py

+            for contrib in submitted_contributions:
+                paper_change_state(contrib, contrib_data, judge=session.user, **judgment_data)
+            num_judged_contribs = len(submitted_contributions)
+            num_prejudged_contribs = len(self.contributions) - num_judged_contribs


I don't think this has anything to do with the paper having been prejudged. Rather, it's simply contributions that haven't received a paper submission. What about renaming the variable num_missing_paper?

OmeGak · 2017-01-09T15:23:07Z

indico/modules/events/papers/operations.py

+        contribution.paper_last_revision.state = PaperRevisionState.rejected
+    elif judgment == PaperAction.to_be_corrected:
+        contribution.paper_last_revision.state = PaperRevisionState.to_be_corrected
+    PaperReviewComment(paper_revision=contribution.paper_last_revision, user=judge,


I also thought we settled for adding the judgment comment as a field in the revision. Not that I disagree with this approach, but just wondering if I missed something.

ThiefMaster · 2017-01-10T16:00:49Z

indico/modules/events/papers/operations.py

@@ -99,8 +98,7 @@ def judge_paper(contribution, contrib_data, judgment, judge, send_notifications=
    elif judgment == PaperAction.to_be_corrected:
        contribution.paper_last_revision.state = PaperRevisionState.to_be_corrected
    if contrib_data['judgment_comment']:


I don't think this check is needed - assigning an empty comment does no harm

ThiefMaster · 2017-01-10T16:13:52Z

indico/htdocs/js/indico/modules/papers.js

@@ -15,7 +15,26 @@
 * along with Indico; if not, see <http://www.gnu.org/licenses/>.
 */

-(function() {
+/* global setupListGenerator:false, applySearchFilters:false, setupTableSorter: false */


no space after the last : for consistency

ThiefMaster · 2017-01-10T16:16:20Z

indico/modules/events/papers/operations.py

+
+@no_autoflush
+def judge_paper(contribution, contrib_data, judgment, judge, send_notifications=False):
+


there shouldn't be an empty line here

ThiefMaster · 2017-01-10T16:16:55Z

indico/modules/events/papers/operations.py

+        contribution.paper_last_revision.state = PaperRevisionState.rejected
+    elif judgment == PaperAction.to_be_corrected:
+        contribution.paper_last_revision.state = PaperRevisionState.to_be_corrected
+    if contrib_data['judgment_comment']:


you can always set the comment. that way judging it again (even though this shouldn't happen) would properly overwrite an existing comment

ThiefMaster · 2017-01-10T16:18:55Z

indico/modules/events/papers/operations.py

+    logger.info('Paper %s changed state by %r to %s', contribution.title, judge,
+                orig_string(judgment.title))
+    contribution.event_new.log(EventLogRealm.management, EventLogKind.change, 'Papers',
+                               'Paper "{}" changed state'.format(contribution.title), judge, data=log_data)


".. was judged .." and I'd include the friendly id in this log message and similar ones: Paper #123 ("foo") was judged...

Might be a good idea to add this property to Contribution (we also have it in Abstract) to make it easier/cleaner:

@property def verbose_title(self): return '#{} ({})'.format(self.friendly_id, self.title)

use orig_string() here, too. we don't want translated messages in logs

ThiefMaster · 2017-01-10T16:19:28Z

indico/modules/events/papers/operations.py

+    log_data = {'New state': orig_string(judgment.title), 'Sent notifications': send_notifications}
+    if send_notifications:
+        pass  # TODO: manage notifications
+    logger.info('Paper %s changed state by %r to %s', contribution.title, judge,


".. was judged .."

Paper %r .. and use contribution instead of contribution.title.
This is for the logfile, having the repr is much more useful there than just the title

and allow to hide these columns

caused by flexbox

OmeGak · 2017-01-11T15:27:06Z

indico/modules/events/papers/lists.py

+        session_empty = {None: _('No session')}
+        type_empty = {None: _('No type')}
+        state_choices = {state.value: state.title for state in PaperRevisionState}
+        track_choices = OrderedDict((unicode(t.id), t.title) for t in sorted(self.event.tracks,


Out of curiosity, why do we need to unicode the IDs?

most likely it goes through json encoding at some point and JSON only supports string keys

mvidalgarcia force-pushed the paper-assignment-page branch from c4d3194 to 8daf1ed Compare December 9, 2016 14:46

nop33 self-assigned this Dec 9, 2016

nop33 suggested changes Dec 9, 2016

View reviewed changes

nop33 removed their assignment Dec 9, 2016

nop33 suggested changes Dec 9, 2016

View reviewed changes

ThiefMaster reviewed Dec 9, 2016

View reviewed changes

mvidalgarcia changed the title ~~Paper assignment page~~ WIP Paper assignment page Dec 12, 2016

OmeGak reviewed Dec 12, 2016

View reviewed changes

OmeGak force-pushed the devel branch from 7a2b43d to 5c9c2c1 Compare December 14, 2016 16:58

nop33 force-pushed the devel branch from 5c9c2c1 to 51c0d10 Compare December 14, 2016 17:04

mvidalgarcia force-pushed the paper-assignment-page branch 3 times, most recently from 0653c09 to 9521130 Compare December 15, 2016 16:15

nop33 suggested changes Dec 16, 2016

View reviewed changes

mvidalgarcia force-pushed the paper-assignment-page branch from 7a82af3 to e131297 Compare December 16, 2016 13:35

ThiefMaster reviewed Dec 16, 2016

View reviewed changes

pferreir reviewed Dec 16, 2016

View reviewed changes

mvidalgarcia force-pushed the paper-assignment-page branch from e131297 to 3f4ef8f Compare December 16, 2016 15:46

pferreir approved these changes Dec 16, 2016

View reviewed changes

mvidalgarcia force-pushed the paper-assignment-page branch 2 times, most recently from ac458b7 to 0d6983d Compare December 16, 2016 16:06

ThiefMaster reviewed Dec 16, 2016

View reviewed changes

mvidalgarcia force-pushed the paper-assignment-page branch from 0d6983d to df9f4df Compare December 16, 2016 16:26

nop33 suggested changes Dec 18, 2016

View reviewed changes

mvidalgarcia force-pushed the paper-assignment-page branch from df9f4df to 6a2f447 Compare December 19, 2016 09:04

ThiefMaster requested changes Dec 21, 2016

View reviewed changes

nop33 approved these changes Jan 5, 2017

View reviewed changes

OmeGak reviewed Jan 9, 2017

View reviewed changes

mvidalgarcia force-pushed the paper-assignment-page branch from 75c11d7 to 5e37ee0 Compare January 10, 2017 14:15

ThiefMaster reviewed Jan 10, 2017

View reviewed changes

ThiefMaster requested changes Jan 10, 2017

View reviewed changes

OmeGak force-pushed the devel branch from e358411 to 1771b3d Compare January 10, 2017 16:27

mvidalgarcia force-pushed the paper-assignment-page branch 2 times, most recently from cbc6595 to f417061 Compare January 11, 2017 10:43

mvidalgarcia added 2 commits January 11, 2017 15:53

Add paper assignment page

d7fc0a1

Add paper_last_revision relationship

35f4458

mvidalgarcia force-pushed the paper-assignment-page branch from f417061 to d0bba5e Compare January 11, 2017 14:53

mvidalgarcia added 6 commits January 11, 2017 15:56

Add list generator and filter by state

21496eb

Filter by track, session and contribution type

7d38ead

and allow to hide these columns

Fix problem with spacing in filter-statistics

d71b6b3

caused by flexbox

Add bulk actions to change paper state

e0fc244

Add judgment_comment to Revision model

6060009

Make use of judgment comment field

66779c8

mvidalgarcia force-pushed the paper-assignment-page branch from d0bba5e to 66779c8 Compare January 11, 2017 14:56

OmeGak changed the title ~~WIP Paper assignment page~~ Paper assignment page Jan 11, 2017

OmeGak reviewed Jan 11, 2017

View reviewed changes

OmeGak merged commit 69b2875 into indico:devel Jan 11, 2017

mvidalgarcia deleted the paper-assignment-page branch January 12, 2017 09:51



		def paper_change_state(contribution, contrib_data, judgment, judge, send_notifications=False):
		log_data = {'New state': orig_string(judgment.title)}


		@no_autoflush
		def judge_paper(contribution, contrib_data, judgment, judge, send_notifications=False):

Paper assignment page #2756

Paper assignment page #2756

Conversation

mvidalgarcia commented Dec 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster Dec 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvidalgarcia commented Dec 9, 2016 •

edited

ThiefMaster Dec 16, 2016 •

edited