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

Reviewing questions #3175

Merged
merged 20 commits into from Jan 17, 2018

Conversation

Projects
None yet
3 participants
@mic4ael
Member

mic4ael commented Dec 7, 2017

No description provided.

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch from 55459a7 to 4d0c980 Dec 7, 2017

@declared_attr
def question_type(cls):
return db.Column(
db.Text,

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

db.String; we usually only use Text for multiline stuff. technically it makes no real difference though since varchar/text in postgres are almost the same

This comment has been minimized.

@pferreir

pferreir Dec 7, 2017

Member

Maybe this could be an enum?

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

Not a great idea IMO, since that'd make it much harder if we ever wanted to use a signal to return the field types (like we do in surveys, contrib fields, etc) instead of supporting only a hardcoded list.

This comment has been minimized.

@pferreir

pferreir Dec 8, 2017

Member

Ah, you mean having it extended by plugins? Good point.

@property
def description(self):
return None

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

maybe add a comment that this is required by BaseField?

@property
def is_required(self):
return self.field_data['is_required']

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

I don't think this should be in field_data. Stuff that is in common_settings is assumed to be in a column, not the field-specific data: (see update_object in BaseField)

This comment has been minimized.

@mic4ael

mic4ael Dec 7, 2017

Member

ok, fair enough, I didn't notice that method

schema='event_abstracts')
op.alter_column('abstract_review_questions', 'question_type', server_default=None, schema='event_abstracts')
op.add_column('abstract_review_questions', sa.Column('field_data', sa.JSON(), nullable=False,
server_default='{"is_required": true}'),

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

see my comment above; this should be in a column

schema='event_paper_reviewing')
op.alter_column('review_questions', 'question_type', server_default=None, schema='event_paper_reviewing')
op.add_column('review_questions', sa.Column('field_data', sa.JSON(), nullable=False,
server_default='{"is_required": true}'),

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

see my comment above; this should be in a column

raise NotFound
class RHCreateReviewingQuestion(RHReviewingQuestionsActionsBase):

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

I think most of this logic could be moved into mixins to avoid duplicating it for abstracts/papers...

This comment has been minimized.

@mic4ael

mic4ael Dec 8, 2017

Member

extracted to functions which are in operations.py module

@@ -236,6 +216,10 @@ def split_data(self):
return {'questions_data': {k: v for k, v in data.iteritems() if k.startswith('question_')},
'review_data': {k: v for k, v in data.iteritems() if not k.startswith('question_')}}
@property
def has_questions(self):
return len(filter(lambda item: item[0].startswith('question_'), self.data.iteritems())) != 0

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

see my comment on the other incarnation of this method

@property
def field_data(self):
return {'is_required': self.is_required.data}

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

shouldn't be in field_data

config_form_base = TextReviewingQuestionConfigForm
def get_reviewing_question_types():

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

Why not add an argument for the type (abstracts/papers) instead of returning both?

{% elif rating.question.question_type == 'bool' %}
{{ _('Yes') if rating.value else _('No') }}
{% else %}
{{ rating.value|safe }}

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 7, 2017

Member

|safe? that sounds like a very bad idea since it lets you put HTML there...

@declared_attr
def question_type(cls):
return db.Column(
db.Text,

This comment has been minimized.

@pferreir

pferreir Dec 7, 2017

Member

Maybe this could be an enum?

@@ -335,6 +324,10 @@ def split_data(self):
return {'questions_data': {k: v for k, v in data.iteritems() if k.startswith('question_')},
'review_data': {k: v for k, v in data.iteritems() if not k.startswith('question_')}}
@property
def has_questions(self):
return len(filter(lambda item: item[0].startswith('question_'), self.data.iteritems())) != 0

This comment has been minimized.

@pferreir

pferreir Dec 7, 2017

Member

👍 for the suggestion, 👎 for the monkey. Nothing against monkeys.

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch 6 times, most recently from a5bca2b to f36707a Dec 7, 2017

@mic4ael

This comment has been minimized.

Member

mic4ael commented Dec 8, 2017

Updated

@property
def description(self):
"""Required by BaseField for creating an instance of WTForms field"""

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

I think this should be a comment, not a docstring

def create_reviewing_question(event, question_model, wtf_field_cls, form, data=None):
new_question = question_model()
new_question.no_score = True if wtf_field_cls.name != 'rating' else getattr(form, 'no_score', True)

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

wtf_field_cls.name != 'rating' or getattr(form, 'no_score', True)

Also, if form is a wtform, then the else block is broken, since the attribute is a field, not its data.

@property
def no_score(self):
return self.field_type != 'rating' or self._no_score

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

You might want to add a .expression for the property; like this you can use it in filter() criteria as well.

.join(AbstractReviewRating.review)
.join(AbstractReviewRating.question)
.filter(AbstractReview.abstract == self,
AbstractReviewQuestion.field_type == 'rating',
AbstractReviewRating.value.cast(db.String) != db.text("'null'"),

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

I'd still change the field type to JSONB and use jsonb_typeof for the check.

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

why jsonb?

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

Because there is no json_typeof. JSON is stored as a string, while JSONB is stored in a binary format that allows postgres to actually look at its contents.

This comment has been minimized.

@mic4ael

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

Indeed, looks like it's only the operators that are JSONB-only. Anyway, the performance is probably better with JSONB

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

Yeah, but do we need that performance in a field which holds only one simple value?

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

No, it probably doesn't make a noticeable difference here.

"IN(SELECT id FROM event_paper_reviewing.review_questions WHERE field_type != 'rating')")
op.execute("DELETE FROM event_paper_reviewing.review_questions WHERE field_type != 'rating'")
op.execute('ALTER TABLE event_paper_reviewing.review_ratings ALTER COLUMN "value" TYPE INT '
'USING to_json(value)::TEXT::INT')

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

Why to_json()? value is already JSON here, isn't it?

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

Not sure why.. my overlooking

if old_value != rating.value:
changes[field_name] = (old_value, rating.value)
changes[field_name] = (question.field.get_friendly_value(old_value),
question.field.get_friendly_value(rating.value))
log_fields[field_name] = {
'title': question.text,
'type': 'number'

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

needs to depend on the question type

return {'coerce': int, 'choices': choices, 'rating_range': range_, 'question': self.object}
class BoolReviewingQuestionConfigForm(BaseReviewingQuestionConfigForm):

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

Why not just use the base one? this subclass seems to be a bit pointless

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

yeah, I just wanted to have separate classes for each field

class BaseReviewingQuestionConfigForm(IndicoForm):
text = StringField(_('Question'), [DataRequired()])
is_required = BooleanField(_('Required'), widget=SwitchWidget())

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

can we avoid replicating all the config forms here and just use the ones of the base fields? i think you are only replicating them to get rid of description but keeping a description wouldn't be so bad IMO! (on the review page we could show it as an info tooltip for example)

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

This title = StringField(_('Title'), [DataRequired()], description=_("The title of the field")) from FieldConfigForm makes a little bit of a problem. Since in the context of reviewing fields Title for the field label is wrong.

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

"[Question] Title" is not so bad IMO, and you could modify the description after instantiating the form

{% for name, field_type in field_types.iteritems() -%}
<li>
<a class="js-action-button"
data-href="{{ url_for(actions.create, event, field_type=name, **(args.create|default({}))) }}"

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

if you get rid of the nested dict as suggested above, you can just use **common_url_args instead of the default({}) stuff

data-href="{{ url_for(actions.create, event, field_type=name, **(args.create|default({}))) }}"
data-title="{% trans %}Add custom reviewing question{% endtrans %}"
data-ajax-dialog>
{% trans name=field_type.friendly_name %}

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 8, 2017

Member

what's the point of this trans block?

This comment has been minimized.

@mic4ael

mic4ael Dec 11, 2017

Member

Ah, indeed, the friendly_name is already translated here.

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 11, 2017

Member

Even if it was, this wouldn't help, since the string would not be extracted for translation (it might just work by coincidence if it was extracted somewhere else)

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch 5 times, most recently from b58dcea to 3ca962d Dec 10, 2017

@mic4ael

This comment has been minimized.

Member

mic4ael commented Dec 11, 2017

Updated

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch from 3ca962d to 47de067 Dec 11, 2017

@no_score.expression
def no_score(self):
return self._no_score

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

I think we want the proper check here (also, rename self to cls since it's the class and not an instance):

return (cls.field_type != 'rating') | cls._no_score
def title(self):
return self.text
# Required by BaseField for creating an instance of WTForms field

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

I would move this inside the method

def upgrade():
op.add_column('abstract_review_questions',

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

If the code is the same for abstract and paper review questions, then I'd use a loop instead of repeating the code:

for schema, table in (('x', 'y'), ('a', 'b')):
class BaseReviewingQuestionConfigForm(IndicoForm):
text = StringField(_('Question'), [DataRequired()])
is_required = BooleanField(_('Required'), widget=SwitchWidget())

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

"[Question] Title" is not so bad IMO, and you could modify the description after instantiating the form

log_fields[field_name] = {
'title': question.text,
'type': 'number'
'type': question.field_type

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

I don't think we want 'type': 'rating' here, since it's just a number in the end.

@@ -89,7 +89,11 @@ def render_changes(a, b, type_):
:param b: new value
:param type_: the type determining how the values should be compared
"""
if type_ in ('number', 'enum', 'bool', 'datetime'):
if type_ in ('number', 'enum', 'bool', 'datetime', 'rating'):

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

see my comment above, i would not introduce a 'rating' type here

new_question = question_model()
new_question.no_score = True
if 'no_score' in form.data:
new_question.no_score = form.data['no_score']

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

if form.no_score: and (in the next line) form.no_score.data. no need to build the data dict thrice

This comment has been minimized.

@mic4ael

mic4ael Dec 13, 2017

Member

form.no_score will raise an AttributeError if there is no such a property

This comment has been minimized.

@mic4ael

mic4ael Dec 13, 2017

Member

if hasattr(form, 'no_score'): will be a better option

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 13, 2017

Member

You can use 'no_score' in form

log_fields[field_name] = {
'title': question.text,
'type': 'number'
'type': question.field_type

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

see my other comment

return $(item).data('item-id');
});
ids = $.makeArray(ids);

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

If this is just converting the jquery object ids to a plain array, you can just call .get() after .map()

{% for bullet in range(rating_range[1], rating_range[0] - 1, -1) %}
{% set radio_id = '%s-%d'|format(field.id, bullet) %}
<input type="radio" id="{{ radio_id }}" name="{{ field.name }}" value="{{ bullet }}"
{{ 'checked' if field.data == bullet }}>

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 12, 2017

Member

1 space too much indentation

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch 3 times, most recently from 85bbcb3 to 652a5fb Dec 13, 2017

@@ -19,6 +19,7 @@
def upgrade():
for schema, a, b in (('event_abstracts', 'abstract_review_ratings', 'abstract_review_questions'),

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 13, 2017

Member

a and b, really? ;)

how about some meaningful names. You can always define the list in a separate statement above to keep it readable.

@@ -41,7 +41,7 @@
</div>
{%- endmacro %}
{% macro form_field(field, field_classes, widget_attrs={}, disabled=none) -%}
{% macro form_field(field, field_classes, hide_description, widget_attrs={}, disabled=none) -%}

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 13, 2017

Member

this should default to false

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch from 5daaee0 to 811d932 Dec 13, 2017

@mic4ael

This comment has been minimized.

Member

mic4ael commented Dec 13, 2017

Updated

@ThiefMaster ThiefMaster added this to the v2.1 milestone Dec 14, 2017

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch from 811d932 to 377ee4a Jan 8, 2018

@ThiefMaster ThiefMaster changed the base branch from v2.1-dev to master Jan 12, 2018

@mic4ael mic4ael force-pushed the mic4ael:reviewing-questions branch from 377ee4a to 30dc3a6 Jan 16, 2018

@ThiefMaster ThiefMaster force-pushed the mic4ael:reviewing-questions branch from 30dc3a6 to 90493f8 Jan 17, 2018

@ThiefMaster ThiefMaster merged commit 705b17a into indico:master Jan 17, 2018

1 check was pending

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

@mic4ael mic4ael deleted the mic4ael:reviewing-questions branch Jan 18, 2018

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