Code review comment for lp:~cprov/uci-engine/webui-create-ticket

On Tue, Sep 23, 2014 at 8:14 AM, Evan Dandrea
<email address hidden> wrote:
> Review: Needs Fixing
>
> Comments inline. Are we going to handle form validation in a follow up MP?

Basic validation were already in place (required fields). I've added
an email-address validation for the current owner field for now, but
once authentication is in place that field will be removed/hidden.

> Title, description, and owner end up as clickable links that focus on the title input box. Series is plain text. This looks odd.

I've extended and fixed Y.SelectField() which was being stupid when
rendering the label.

>> + {name: 'title', required: true, label: 'Title'},
>> + {name: 'description', label: 'Description',
>> + type: 'TextareaField'},
>> + {name: 'owner', required: true, label: 'Owner'},
>> + {name : 'series', required: true, label : 'Series',
>> + type : 'SelectField',
>> + choices : [
>> + {label : 'Utopic', value : 'utopic'},
>> + {label : 'Trusty', value : 'trusty'},
>> + {label : 'Precise', value : 'precise'},
>
> Must we hardcode this? If so, I think we should follow up with an MP to put this in a separate js file that can be pulled in from any code that needs it.

I've split the 'webuiform' module as an experiment, overall loading
performance is faster. 'webuiforms' also host 'seriesChoices', so it's
easier to maintain the hardcoded bits until there is a better
solution.

>> + ]},
>> + {name: 'create-button', type: 'SubmitButton',
>> + value: 'Create!'}
>> + ]
>> + });
>> + f.render();
>> +
>> + // Hijack the default (www-urlencoded) form submission and
>> + // submit its content as JSON.
>> + f.getField('create-button').on('click', function(e) {
>> + e.halt();
>> + if (!f._runValidation()) {
>> + return 1;
>> + }
>> +
>> + // XXX cprov 20140922: extend the form data with appropriate
>> + // 'status' and 'current_workflow_step'.
>> + var form_data = f.toJSON();
>> +
>> + Y.io(f.get('action'), {
>> + method: f.get('method'),
>> + sync: true,
>> + data: Y.JSON.stringify(form_data),
>> + headers: {
>> + 'Content-Type': 'application/json;',
>> + },
>> + on: {
>> + success: function(tx, r) {
>> + var location = r.getResponseHeader('Location');
>> + Y.log('SUCCESS: ' + location);
>
> Leftover from debugging?

Yes, removed.

>> + Y.config.win.location.href = location.replace(
>> + '/api/v1', '');
>> + },
>> + failure: function (tx, r) {
>> + Y.log('FAILURE: ' + r.responseText);
>> + f.getField('create-button').set(
>> + 'error', 'Ticket creation failed!')
>
> A weird place to put an error message. We haven't disabled the button either, so if I want to try again I fill out the form and hit 'Ticket creation failed!'. Could we not have a <p> set aside for errors?

Form failures are displayed in a balloon-like area above the form.
Style is still controversial, suggestions are welcome.

[]

--
Celso Providelo
<email address hidden>