[Toaster] [review-request] adamian/20150603_remove_xhr_review2 - REST refactoring

Damian, Alexandru alexandru.damian at intel.com
Wed Jun 10 03:44:10 PDT 2015 

Hello,

I've tried to refactor the patch based on the remarks below, I have run
into problems.

I will comment on each item below, but as to move things forward, I will
submit this patchset with some minor fixes to issues you spotted below, and
we can tackle the rest of issues on a subsequent patch.

Cheers,
Alex

On Tue, Jun 9, 2015 at 5:05 PM, Michael Wood <michael.g.wood at intel.com>
wrote:

>
>
> d3faa0ec50d84f3f7893e7c88b40993b021b4707 <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=adamian/20150603_remove_xhr_review2&id=d3faa0ec50d84f3f7893e7c88b40993b021b4707
> >
>
>
> diff --git a/bitbake/lib/toaster/toastergui/static/js/table.js
> b/bitbake/lib/toaster/toastergui/static/js/table.js
> index 7588a4a..80e9ec2 100644
> --- a/bitbake/lib/toaster/toastergui/static/js/table.js <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/static/js/table.js?h=adamian/20150603_remove_xhr_review2&id=6a9a3995803334ef6a9b9eb238855f5bdb827759
> >
> +++ b/bitbake/lib/toaster/toastergui/static/js/table.js <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/static/js/table.js?h=adamian/20150603_remove_xhr_review2&id=d3faa0ec50d84f3f7893e7c88b40993b021b4707
> >
> @@ -113,8 +113,20 @@ function tableInit(ctx){
>     for (var i in tableData.rows){
>         var row = $("<tr></tr>");
>         for (var key_j in tableData.rows[i]){
> +     /* if we have a static: version of a key, prefer the static: version
> for rendering */
> +     var orig_key_j = key_j;
>
>
> I'm not keen on having data for the table widget and data for the API
> mixed up in the same table/data response. A different definition of table
> should be used if it's for a different purpose.
>

​I agree that this compromise is not ideal - the server should not ever
send HTML to the frontend - it should send raw data and have the frontend
render it locally - this is because we tie a specific front-end
representation of data to a backend implementation.

Until we have enough code in the frontend to allow us to get rid of the
server-side HTML rendering, I would say that the compromise of moving
server-side render in its own namespace is acceptable for now.​


>
> diff --git a/bitbake/lib/toaster/toastergui/views.py
> b/bitbake/lib/toaster/toastergui/views.py
> index 91c4fa2..280159a 100755
> --- a/bitbake/lib/toaster/toastergui/views.py
> +++ b/bitbake/lib/toaster/toastergui/views.py
> @@ -87,6 +87,35 @@ def _project_recent_build_list(prj):
> list(prj.buildrequest_set.filter(state__in=[BuildRequest.REQ_COMPLETED,
> BuildRequest.REQ_FAILED]).order_by("-pk")[:3]))
>
>
> +
> +def objtojson(obj):
>
> Could you comment on this function it's not obvious why it's needed or
> what it does
>

​Django objects are not generally serializable to JSON in ​a
straightforward fashion. This is a helper function that serialize Toaster
objects in a way that it provides enough information for a consumer to make
the JSON representation usable.


>
>
> 732d175ded8517cd6dee02d441f318c775aaba8b <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=adamian/20150603_remove_xhr_review2&id=732d175ded8517cd6dee02d441f318c775aaba8b
> >
>
> diff --git a/bitbake/lib/toaster/toastergui/widgets.py
> b/bitbake/lib/toaster/toastergui/widgets.py
> index 82b7514..407a0fb 100644
> --- a/bitbake/lib/toaster/toastergui/widgets.py <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/widgets.py?h=adamian/20150603_remove_xhr_review2&id=d3faa0ec50d84f3f7893e7c88b40993b021b4707
> >
> +++ b/bitbake/lib/toaster/toastergui/widgets.py <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/widgets.py?h=adamian/20150603_remove_xhr_review2&id=732d175ded8517cd6dee02d441f318c775aaba8b
> >
> ...
>
> @@ -289,8 +299,11 @@ class ToasterTable(TemplateView):
> col['field_name'] = col['static_data_name']
> - if True: # we add the raw model data at all times
> - model_data = row
> + # compute the computation on the raw data if needed
> + model_data = row
> + if col['computation']:
>     + model_data = col['computation'](row)
> + else:
>
>
> I don't see why there should be a case where you're computing data on each
> row, if you're having to do this then I think the model is wrong.
>
> couldn't this be done with a filter?
>

​This is needed because the row may contain data that is not directly
mapped to a field representation of the database. For example, the
particular column​ for which the value was computed is the layer version
for the recipe in context of the project. This not solvable using a filter.



>
> + self.add_column(title="Project compatible Layer ID",
> + displayable = False,
> + field_name = "projectcompatible_layer",
> + computation = lambda x:
> (x.layer_version.get_equivalents_wpriority(Project.objects.get(pk=kwargs['pid']))[0]))
> +
>
> It's pretty confusing when looking at the JSON response, you get lots of
> other objects included even though you asked for recipe objects, infact I
> counted 16 different objects referenced in the JSON response for recipes
> (e.g. http://127.0.0.1:9999/toastergui/project/2/recipes/?format=json )
> I'm not sure why.
>
>
>
> bb507c35ae1af626681dcb666491f6ea97ea31f5 <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=adamian/20150603_remove_xhr_review2&id=bb507c35ae1af626681dcb666491f6ea97ea31f5
> >
>
>
> diff --git a/bitbake/lib/toaster/toastergui/widgets.py
> b/bitbake/lib/toaster/toastergui/widgets.py
> index 8bc3d7f..f5a1b3e 100644
> --- a/bitbake/lib/toaster/toastergui/widgets.py <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/widgets.py?h=adamian/20150603_remove_xhr_review2&id=73d734fd193b2fe591b9407038da5d7d6db0fabf
> >
> +++ b/bitbake/lib/toaster/toastergui/widgets.py <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/widgets.py?h=adamian/20150603_remove_xhr_review2&id=bb507c35ae1af626681dcb666491f6ea97ea31f5
> >
> @@ -253,8 +253,8 @@ class ToasterTable(TemplateView):
> data = cache.get(cache_name)
> - if data:
> - return data
> + #if data:
> + # return data
>
>
​Disabled for testing, thanks for spotting this. I'll re-enable.​


>
> ?
>
>
> diff --git a/bitbake/lib/toaster/toastermain/settings.py
> b/bitbake/lib/toaster/toastermain/settings.py
> index 225138b..3c7cb3b 100644
> --- a/bitbake/lib/toaster/toastermain/settings.py <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastermain/settings.py?h=adamian/20150603_remove_xhr_review2&id=73d734fd193b2fe591b9407038da5d7d6db0fabf
> >
> +++ b/bitbake/lib/toaster/toastermain/settings.py <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastermain/settings.py?h=adamian/20150603_remove_xhr_review2&id=bb507c35ae1af626681dcb666491f6ea97ea31f5
> >
> @@ -223,7 +223,7 @@ CACHES = {
> 'default': {
> 'BACKEND': 'django.core.cache.backends.filebased.FileBasedCache',
> 'LOCATION': '/tmp/django-default-cache',
> - 'TIMEOUT': 5,
> + 'TIMEOUT': 1,
> }
> }
>
> 1 second seems a bit too short to me..
>

​Likewise, for testing, should've not been comitted.​


>
>
> a52772e243680d46836555b3eec805b7eb1d66d4 <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=adamian/20150603_remove_xhr_review2&id=a52772e243680d46836555b3eec805b7eb1d66d4
> >
>
>
> diff --git a/bitbake/lib/toaster/toastergui/static/js/base.js
> b/bitbake/lib/toaster/toastergui/static/js/base.js
> index 747442c..5efe458 100644
> --- a/bitbake/lib/toaster/toastergui/static/js/base.js
> +++ b/bitbake/lib/toaster/toastergui/static/js/base.js
> @@ -21,7 +21,15 @@ function basePageInit (ctx) {
>
> +
> +          /* we can build this project; enable input fields */
> +          newBuildTargetInput.prop("disabled", false);
> +          newBuildTargetBuildBtn.prop("disabled", false);
> +
> +          libtoaster.makeTypeahead(newBuildTargetInput,
> libtoaster.ctx.projectTargetsUrl, { format: "json" }, function(item){
> +            /* successfully selected a target */
> +            selectedTarget = item;
> +            });
> +
>          }
>      }, null);
>
>
> Minor white space issues here.
>
> 31f7cef51041d7244dbf95a5258e45cbbf851f43 <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=adamian/20150603_remove_xhr_review2&id=31f7cef51041d7244dbf95a5258e45cbbf851f43
> >
>
> diff --git a/bitbake/lib/toaster/toastergui/static/js/base.js
> b/bitbake/lib/toaster/toastergui/static/js/base.js
> index ccc23e0..9424b6c 100644
> --- a/bitbake/lib/toaster/toastergui/static/js/base.js <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/static/js/base.js?h=adamian/20150603_remove_xhr_review2&id=c93f33414004d6e6f2040c226ac3488d5f8c6125
> >
> +++ b/bitbake/lib/toaster/toastergui/static/js/base.js <
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/tree/bitbake/lib/toaster/toastergui/static/js/base.js?h=adamian/20150603_remove_xhr_review2&id=31f7cef51041d7244dbf95a5258e45cbbf851f43
> >
> @@ -15,20 +15,21 @@ function basePageInit (ctx) {
>
>
> +      /* we set the effective context of the page to the currently
> selected project */
> +      /* TBD: do we override even if we already have a context project ??
> */
> +      /* TODO: replace global library context with references to the
> "selected" project */
> +      libtoaster.ctx.projectPageUrl = selectedProject.projectPageUrl;
> +      libtoaster.ctx.xhrProjectEditUrl =
> selectedProject.xhrProjectEditUrl;
> +      libtoaster.ctx.projectName = selectedProject.name;
> +      libtoaster.ctx.projectId = selectedProject.id;
>
>
> This creates strange behaviour if you're on a page which has actions such
> as build or add/remove layer. If you're on project 1 and open the New build
> button and select a project, suddenly all the buttons on the page you're on
> act on a different project from the one you're viewing.
>

​Ok, I will solve it in a subsequent patch.​



>
> Michael
>
>
>
> On 09/06/15 11:58, Barros Pena, Belen wrote:
>
>> Sorry: caching playing tricks on me.
>>
>> All is working, except
>>
>> * In the all compatible recipes page, when we show a commit in the
>> Revision column, the full SHA is showing, instead of the button with the
>> first characters of it. This is working properly in the all layers page,
>> by the way.
>>
>>
>> Which we can fix at a later stage.
>>
>> Thanks!
>>
>> Belén
>>
>> On 09/06/2015 11:02, "Barros Pena, Belen" <belen.barros.pena at intel.com>
>> wrote:
>>
>>  I've changed the subject to the branch we need to review, so that we
>>> don't
>>> get confused.
>>>
>>> On 08/06/2015 18:44, "Damian, Alexandru" <alexandru.damian at intel.com>
>>> wrote:
>>>
>>>  Sent too soon -
>>>>
>>>>
>>>> adamian/20150603_remove_xhr_review2
>>>>
>>> There are still a few things not working for me:
>>>
>>> * The autocomplete functionality is still not working for me in the
>>> project page, but it seems to work in the 'new build' button
>>>
>>> * In the all compatible recipes page, when we show a commit in the
>>> Revision column, the full SHA is showing, instead of the button with the
>>> first characters of it. This is working properly in the all layers page,
>>> by the way.
>>>
>>> * Builds are not starting: I've tried from several pages, but when I
>>> click
>>> the build button, the build does not start.
>>>
>>> * I cannot change the project release: when I click the 'save' button I
>>> get the following error
>>>
>>> Release matching query does not exist.
>>> Traceback (most recent call last):
>>>   File "/home/yocto/master/bitbake/lib/toaster/toastergui/views.py", line
>>> 2337, in xhr_datatypeahead
>>>     lv = prj.compatible_layerversions(release =
>>> Release.objects.get(pk=request.GET.get('search',
>>> None))).filter(layer__name = i.layercommit.layer.name)
>>>   File
>>> "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py",
>>> line
>>> 151, in get
>>>     return self.get_queryset().get(*args, **kwargs)
>>>   File
>>> "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py",
>>> line 307, in get
>>>     self.model._meta.object_name)
>>> DoesNotExist: Release matching query does not exist
>>>
>>> Thanks!
>>>
>>> Belén
>>>
>>>
>>>
>>>>
>>>> Can you guys please review ?
>>>>
>>>>
>>>> If ok, I'm going to submit.
>>>>
>>>>
>>>> Alex
>>>>
>>>>
>>>> On Mon, Jun 8, 2015 at 6:43 PM, Damian, Alexandru
>>>> <alexandru.damian at intel.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> I think I'm done with the URL refactoring - the patchset is here
>>>>
>>>>
>>>>
>>>>
>>>> On Thu, Jun 4, 2015 at 10:25 AM, Barros Pena, Belen
>>>> <belen.barros.pena at intel.com> wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> I've had a quick look at this branch. Table headings are back :) but
>>>> found
>>>> a couple of things:
>>>>
>>>> * the layer dependencies check is not working. When I add a layer with
>>>> dependencies, I am not prompted to add them.
>>>> * the autocomplete functionality is not working. I always get the same
>>>> suggestions independently of what I type
>>>> * the new build button in pages belonging to a project shows the recipe
>>>> text field and build buttons disabled, even though there is a project
>>>> selected.
>>>> * trying to change the project release throws the following error:
>>>>
>>>> Release matching query does not exist.
>>>> Traceback (most recent call last):
>>>>   File "/home/yocto/master/bitbake/lib/toaster/toastergui/views.py",
>>>> line 2393, in xhr_datatypeahead
>>>>     lv = prj.compatible_layerversions(release =
>>>> Release.objects.get(pk=request.GET.get('search',
>>>> None))).filter(layer__name =
>>>> i.layercommit.layer.name <http://i.layercommit.layer.name>)
>>>>   File
>>>> "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py",
>>>> line 151, in get
>>>>     return self.get_queryset().get(*args, **kwargs)
>>>>   File
>>>> "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py",
>>>> line 307, in get
>>>>     self.model._meta.object_name)
>>>> DoesNotExist: Release matching query does not exist.
>>>>
>>>> The rest seems to be working ok, as far as I can see.
>>>>
>>>>
>>>> Thanks!
>>>>
>>>> On 03/06/2015 15:35, "Damian, Alexandru" <alexandru.damian at intel.com>
>>>> wrote:
>>>>
>>>>  Thanks -
>>>>>
>>>>>
>>>>> I'm pushing new patches to bring more parts of the system in
>>>>> conformance
>>>>> - just removed xhr_project(info|edit) in favor of JSON-based "project"
>>>>> page.
>>>>>
>>>>>
>>>>> I will update on this thread when I tackled the bugs highlighted on the
>>>>> other thread, and when the new functionality is ready to merge from my
>>>>> POV.
>>>>>
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>>> On Wed, Jun 3, 2015 at 2:59 PM, Michael Wood
>>>>> <michael.g.wood at intel.com> wrote:
>>>>>
>>>>>
>>>>> 8305ee985bdc87dcfb63be4e0ddafb0a9239ffd4
>>>>>
>>>>> Looks good to me.
>>>>>
>>>>> Yeah that's a good idea to make ToasterTable inherit from TemplateView
>>>>> rather than have it in between.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>>
>>>>> On 03/06/15 14:15, Damian, Alexandru wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I've progressed a bit with the REST refactoring, I've tackled the
>>>>> ToasterTables. Thanks to good quality of the existing code , changing
>>>>> the
>>>>> layout of the URL was straightforward.
>>>>>
>>>>> adamian/20150515_fix_table_header
>>>>>
>>>>> This reduces the number of entry points for the URL, removes the ugly
>>>>> hardcoding of XHR_ table urls in tables.py, and makes sure the APIs are
>>>>> both human- and machine-readable.
>>>>>
>>>>> I'll continue with refactoring to remove the xhr_datatypeahead and
>>>>> similar URL endpoints which should not have existed in the first place.
>>>>>
>>>>> Cheers,
>>>>> Alex
>>>>>
>>>>> --
>>>>> Alex Damian
>>>>> Yocto Project
>>>>> SSG / OTC
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> Intel Corporation (UK) Limited
>>>>> Registered No. 1134945 (England)
>>>>> Registered Office: Pipers Way, Swindon SN3 1RJ
>>>>> VAT No: 860 2173 47
>>>>>
>>>>> This e-mail and any attachments may contain confidential material for
>>>>> the sole use of the intended recipient(s). Any review or distribution
>>>>> by others is strictly prohibited. If you are not the intended
>>>>> recipient, please contact the sender and delete all copies.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Alex Damian
>>>>> Yocto Project
>>>>>
>>>>> SSG / OTC
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Alex Damian
>>>> Yocto Project
>>>>
>>>> SSG / OTC
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Alex Damian
>>>> Yocto Project
>>>>
>>>> SSG / OTC
>>>>
>>>>
>>>>
>>>>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>


-- 
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150610/99a4fd0a/attachment-0001.html>

More information about the toaster mailing list