[Toaster] Review required for patches related to bug 7005
Michael Wood
michael.g.wood at intel.com
Mon Apr 25 04:13:51 PDT 2016
Hi Sujith,
Thanks for the patches here is some code-review, it's mostly cosmetic
issues:
http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=sujith/toaster-unique-projectname&id=17739189416e444d8dce7cb7c64aaf776f85d1ff
From 17739189416e444d8dce7cb7c64aaf776f85d1ff Mon Sep 17 00:00:00 2001
From: Sujith H <sujith.h at gmail.com>
Date: Mon, 25 Apr 2016 06:27:35 +0000
Subject: toaster: projectNameValidation API added
The projectNameValidation API would help users
to validate if a project name exists or not. This
API is added to libtoaster.
[YOCTO #7005]
Signed-off-by: Sujith H <sujith.h at gmail.com>
diff --git a/bitbake/lib/toaster/toastergui/static/js/libtoaster.js b/bitbake/lib/toaster/toastergui/static/js/libtoaster.js
index 43930a2..e8c14cd 100644
--- a/bitbake/lib/toaster/toastergui/static/js/libtoaster.js
+++ b/bitbake/lib/toaster/toastergui/static/js/libtoaster.js
@@ -374,6 +374,46 @@ var libtoaster = (function (){
});
}
+ /* Validate project names. Use unique project names */
+ /* Arguments include projectnameid, hinterrorid, validateprojectnameid, enableordisablebtnid */
- MW - Good that you're documenting the function but we need to know
what the arguments are, not just listing the variable names, that
doesn't really help as the names are already below in the function
definition anyway. We need to have a short description of what the
variables are expecting and the type if it's not obvious e.g. string or
jquery Element.
+ function _projectNameValidation(projectnameid, hinterrorid,
+ ctrlgrpvalidateprojectnameid, enableordisablebtnid ) {
+
- MW - The variables are all called somethingid, but you're not passing
an id, you're passing a jQuery element. Either is fine really but make
sure what you're passing in and the variable names match, e.g. if it's
going to be an id string "the-element-id" then it would be something
like projectNameElementId, or if you're going to pass in jQuery object
it could be projectName and then in the documentation string make sure
you say it's a jQuery object expected. Don't forget that in javascript
we use camelCaseLikeThis for variable names and functions.
https://wiki.yoctoproject.org/wiki/Contribute_to_Toaster#Javascript
We should also name the function something more like
makeProjectNameValidation because this function isn't going to return
anything, like 'invalid' or 'valid' as you'd expect from a validation
function, it is in fact setting up/making the event handlers for the inputs.
+ /* The moment user types project name remove the error */
+ projectnameid.on("input", function() {
+ ctrlgrpvalidateprojectnameid.removeClass('control-group error');
+ hinterrorid.hide();
+ });
+
+ /* Validate new project name */
+ projectnameid.on("blur",function() {
+ var projectname = $(this).val();
+ $.ajax({
+ type: "GET",
+ url: libtoaster.ctx.projectsTypeAheadUrl,
+ data: { 'search' : projectname },
+ headers: { 'X-CSRFToken' : $.cookie('csrftoken')},
+ success: function( data ) {
+ if (data.results.length > 0 &&
+ data.results[0].name === projectname) {
+ // This project name exists hence show the error and disable
+ // the save button
+ ctrlgrpvalidateprojectnameid.addClass('control-group error');
+ hinterrorid.show();
+ enableordisablebtnid.attr('disabled', 'disabled');
+ } else {
+ ctrlgrpvalidateprojectnameid.removeClass('control-group error');
+ hinterrorid.hide();
+ enableordisablebtnid.removeAttr('disabled');
+ }
+ },
+ error: function (data) {
+ console.log(data);
+ },
+ });
+ });
+ }
+
- MW - All this needs to be indented by 2 spaces as you're inside the
projectNameValidation function.
return {
reload_params : reload_params,
@@ -390,6 +430,7 @@ var libtoaster = (function (){
makeLayerAddRmAlertMsg : _makeLayerAddRmAlertMsg,
showChangeNotification : _showChangeNotification,
createCustomRecipe: _createCustomRecipe,
+ projectNameValidation: _projectNameValidation,
};
})();
--
cgit v0.10.2
http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=sujith/toaster-unique-projectname&id=8d3b47ec15dbd93e077ec10cd928648258b84181
From 8d3b47ec15dbd93e077ec10cd928648258b84181 Mon Sep 17 00:00:00 2001
From: Sujith H <sujith.h at gmail.com>
Date: Mon, 25 Apr 2016 06:29:19 +0000
Subject: toaster: ui handles duplicate project name in new project page
When already existing project name is typed by user,
the ui pops up message regarding the existance of the
project name.
[YOCTO #7005]
Signed-off-by: Sujith H <sujith.h at gmail.com>
diff --git a/bitbake/lib/toaster/toastergui/templates/newproject.html b/bitbake/lib/toaster/toastergui/templates/newproject.html
index e83b2be..19ab749 100644
--- a/bitbake/lib/toaster/toastergui/templates/newproject.html
+++ b/bitbake/lib/toaster/toastergui/templates/newproject.html
@@ -21,7 +21,10 @@
<fieldset>
<label>Project name <span class="muted">(required)</span></label>
- <input type="text" class="input-xlarge" required id="new-project-name" name="projectname">
+ <div class="control-group" id="validate-project-name">
+ <input type="text" class="input-xlarge" required id="new-project-name" name="projectname">
+ </br><span class="help-block error" style="display: none;" id="hintError-project-name">A project with this name exists. Project names must be unique.</span>
+ </div>
</fieldset>
- MW - hint-error-project-name not hintError-project-name no camel case for HTML elements
https://wiki.yoctoproject.org/wiki/Contribute_to_Toaster#HTML
<!--
<fieldset>
@@ -113,6 +116,13 @@
$('#description-' + new_release).fadeIn();
});
+ try {
+ libtoaster.projectNameValidation($("#new-project-name"), $("#hintError-project-name"), $("#validate-project-name"), $(".btn-primary"));
+ } catch (e) {
+ document.write("Sorry, An error has occurred loading this page");
+ console.warn(e);
+ }
- MW - not sure why you've put this in a try catch? no need for this.
+
/* // Hide the project release when you select an analysis project
function projectType() {
if ($("input[type='radio']:checked").val() == 'build') {
--
cgit v0.10.2
http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=sujith/toaster-unique-projectname&id=3d9d7a628b7280ecb62c7e05dc6a8c5bc0204792
From 3d9d7a628b7280ecb62c7e05dc6a8c5bc0204792 Mon Sep 17 00:00:00 2001
From: Sujith H <sujith.h at gmail.com>
Date: Mon, 25 Apr 2016 06:30:37 +0000
Subject: toaster: ui handles duplicate project name in project page
When already existing project name is typed by user,
the ui pops up message regarding the existance of the
project name. When an existing project is typed the save
button will be disabled. Else user can proceed ahead by
modifying the project name.
[YOCTO #7005]
Signed-off-by: Sujith H <sujith.h at gmail.com>
diff --git a/bitbake/lib/toaster/toastergui/templates/projecttopbar.html b/bitbake/lib/toaster/toastergui/templates/projecttopbar.html
index 007de06..abf598e 100644
--- a/bitbake/lib/toaster/toastergui/templates/projecttopbar.html
+++ b/bitbake/lib/toaster/toastergui/templates/projecttopbar.html
@@ -9,6 +9,7 @@
try {
projectTopBarInit(ctx);
+ libtoaster.projectNameValidation($("#project-name-change-input"), $("#hintError-project-name"), $("#validate-project-name"), $("#project-name-change-btn"));
- MW - This should be inside the projectTopBarInit function in projecttopbar.js and not here. The projectTopBarInit function contains all the setup for the project top bar, setting up the project input is part of that so should go inside the function and not as a top level item. Generally we want as little javascript in the html templates as possible as it makes it much harder to debug and the lines between the templating engine and the client side logic start to get blured
Generally we try also to stick to 80 cols, so if possible wrap this on top more lines.
} catch (e) {
document.write("Sorry, An error has occurred loading this page");
console.warn(e);
@@ -33,11 +34,12 @@
{% endif %}
</h1>
<form id="project-name-change-form" style="margin-bottom: 0px; display: none;">
- <div class="input-append">
+ <div class="input-append" id="validate-project-name">
<input class="huge input-xxlarge" type="text" id="project-name-change-input" autocomplete="off" value="{{project.name}}">
<button id="project-name-change-btn" class="btn btn-large" type="button">Save</button>
<a href="#" id="project-name-change-cancel" class="btn btn-large btn-link">Cancel</a>
</div>
+ <span class="help-block error" style="display: none;" id="hintError-project-name">A project with this name exists. Project names must be unique.</span>
</form>
</div>
--
cgit v0.10.2
Thanks,
Michael
On 19/04/16 15:14, Barros Pena, Belen wrote:
>
> On 18/04/2016 12:53, "sujith h" <sujith.h at gmail.com> wrote:
>
>>
>> On Mon, Apr 18, 2016 at 5:09 PM, sujith h
>> <sujith.h at gmail.com> wrote:
>>
>> Hi Belen,
>>
>> On Mon, Apr 18, 2016 at 4:57 PM, Barros Pena, Belen
>> <belen.barros.pena at intel.com> wrote:
>>
>>
>>
>> On 14/04/2016 14:57, "toaster-bounces at yoctoproject.org on behalf of sujith
>> h" <toaster-bounces at yoctoproject.org on behalf of
>> sujith.h at gmail.com>
>> wrote:
>>
>>>
>>> On Thu, Mar 31, 2016 at 8:39 PM, Michael Wood
>>> <michael.g.wood at intel.com> wrote:
>>>
>>> Done a quick review on IRC we'll try to convert the project page from
>>> doing a POST directly to the view to use the ajax+ReST like approach we
>>> have else where (e.g. custom image recipes).
>>>
>>>
>>> Michael had once again reviewed my patch set today and I had trimmed down
>>> my code:
>>>
>>> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=sujith/toas
>>> t
>>> er-unique-projectname
>>
>> Belen, can you check the branch again. I have deleted the branch and
>> created it again. And now the error doesn't come.
> Yep: error is gone now, so I was able to try it :)
>
> I've found a couple of small issues with the behaviour and the styling:
>
> * we need to apply the .control-group and .error classes to the fieldset
> containing the project name when the error message comes up, so that the
> label and input field look red like the message. That will match what we
> do in the rest of Toaster, and will help people realise there is an issue
> with the name they typed.
>
> * Michael tells me we are using the "change" event, and recommends we use
> "blur" instead
>
> * Finally, the error message should go away the moment the error condition
> disappears, i.e. when users change the name. It needs to work exactly like
> the validation for the layer name field in the import layer page. To see
> it, create a project in Toaster with the master release, then click on
> "import layer", then type "meta-oe" in the layer name and tab or click
> away from the text field. A note will come up saying that a layer with
> that name already exists. Then add / remove something from the layer name:
> on key up the message will go away. This is exactly the behaviour we need
> to use in the case of the project name, making things nicely consistent
> across the UI
>
> Finally, I am afraid my design is incomplete: you can change the name of
> an existing project, so we need to validate the name is unique in that
> case as well. I am really sorry: I should have noticed this. Luckily
> Michael realised for me :)
>
> I've updated the design document attached to the bug
>
> https://bugzilla.yoctoproject.org/show_bug.cgi?id=7005
>
> It now includes a second page explaining the missing bits. Once we have
> things working in the new project page, hopefully it won't be too hard to
> extend the functionality to the project configuration pages.
>
> Thanks!
>
> Belén
>
>>
>>
>>
>>
>> Thanks,
>>
>> Sujith H
>>
>>
>>
>>
>>
>> Thanks!
>>
>> Belén
>>
>>
>>> <http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=sujith/toa
>>> s
>>> ter-unique-projectname>
>>>
>>>
>>>
>>>
>>> Michael
>>>
>>> On 30/03/16 16:04, sujith h wrote:
>>>
>>> Hi,
>>>
>>> I have posted my changes for bug 7005:
>>> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=sujith/toas
>>> t
>>> er-unique-projectname
>>> <http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=sujith/toa
>>> s
>>> ter-unique-projectname> ( The top 2 commits ).
>>>
>>> This patch set helps user not to have same project names for toaster.
>>>
>>> Thanks,
>>> Sujith H
>>>
>>> --
>>> സുജിത് ഹരിദാസന്
>>> Bangalore
>>> <Project>Contributor to KDE project
>>> <Project>Contributor to Yocto project
>>> http://fci.wikia.com/wiki/Anti-DRM-Campaign
>>> <Blog> http://sujithh.info
>>> C-x C-c
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> --
>>> _______________________________________________
>>> toaster mailing list
>>> toaster at yoctoproject.org
>>> https://lists.yoctoproject.org/listinfo/toaster
>>>
>>>
>>>
>>>
>>>
>>>
>>> --
>>> സുജിത് ഹരിദാസന്
>>> Bangalore
>>> <Project>Contributor to KDE project
>>>
>>> <Project>Contributor to Yocto project
>>>
>>> http://fci.wikia.com/wiki/Anti-DRM-Campaign
>>> <Blog> http://sujithh.info
>>>
>>> C-x C-c
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> --
>> സുജിത് ഹരിദാസന്
>> Bangalore
>> <Project>Contributor to KDE project
>>
>> <Project>Contributor to Yocto project
>>
>> http://fci.wikia.com/wiki/Anti-DRM-Campaign
>> <Blog> http://sujithh.info
>>
>> C-x C-c
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> --
>> സുജിത് ഹരിദാസന്
>> Bangalore
>> <Project>Contributor to KDE project
>>
>> <Project>Contributor to Yocto project
>>
>> http://fci.wikia.com/wiki/Anti-DRM-Campaign
>> <Blog> http://sujithh.info
>>
>> C-x C-c
>>
>>
>>
>>
>>
>>
>>
More information about the toaster
mailing list