[review-request] adamian/20150515_fix_table_header

[review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

Hi,

Can you please review this branch ? It brings in an important piece of
refactoring to move Toaster towards the REST-style API that will enable a
cleaner design and support for interconnection with other systems.

Getting into details, the "New build" button in all the pages, bar the
project page, now fetches data using the projects API, which is just the
"all projects" page in machine-readable format. This cleans up a bit the
overloading of the xhr_datatypeahead; after the API refactoring, all xhr_
calls should be completely replaced with proper calls to the REST endpoints.

The code is here:
https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header

Incidentally, this patch fixes the "new build" breakage by the first round
of URL refactoring, and the "construction" of URLs on the client side.

Can you please review and comment ?

Cheers,
Alex

-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 2223 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Michael Wood]

Hi,

Review below.

-
-    libtoaster.makeTypeahead(newBuildProjectInput, { type : "projects" 
}, function(item){
+    libtoaster.makeTypeahead(newBuildProjectInput, 
libtoaster.ctx.projectsUrl, { type : "projects", format : "json" }, 
function(item){
          /* successfully selected a project */
          newBuildProjectSaveBtn.removeAttr("disabled");
          selectedProject = item;
+
+
      })


I understand the having the xhrUrl as a new parameter, but it doesn't 
make any sense to me to have "type" and "format", no one should expect 
setting the type to json as an argument to some magic urls in the 
request would then return something to consume for an API. How are you 
supposed to know which urls support JSON responses?  Really wouldn't be 
happy with that.

If you're wanting to avoid having a query for the projects html page and 
another copy of it for the API response why not use a defined Queryset? 
the django QuerySet managers are exactly for this.



+      /* 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;


What happens if something else accesses those afterwards? If you select 
a project then close the popup you've potentially broken any other 
"user" of that context data.
Ideally those properties are read-only.


+class RedirectException(Exception):
+    def __init__(self, view, g, mandatory_parameters, *args, **kwargs):
+        super(RedirectException, self).__init__()
+        self.view = view
+        self.g = g
+        self.mandatory_parameters = mandatory_parameters
+        self.oargs  = args
+        self.okwargs = kwargs
+
+    def get_redirect_response(self):
+        return _redirect_parameters(self.view, self.g, 
self.mandatory_parameters, self.oargs, self.okwargs)
+

Using HTTP redirects are a real pain because they get cached by the 
browser and are very difficult to invalidate.


-    def projects(request):
-        template="projects.html"
+    def template_renderer(template):
+        def func_wrapper(view):
+            def returned_wrapper(request, *args, **kwargs):
+                try:
+                    context = view(request, *args, **kwargs)
+                except RedirectException as e:
+                    return e.get_redirect_response()
+
+                if request.GET.get('format', None) == 'json':
+                    # objects is a special keyword - it's a Page, but 
we need the actual objects here
+                    # in XHR, the objects come in the "list" property
+                    if "objects" in context:
+                        context["list"] = context["objects"].object_list
+                        del context["objects"]
+
+                    # convert separately the values that are 
JSON-serializable
+                    json_safe = {}
+                    for k in context:
+                        try:
+                            jsonfilter(context[k])
+                            json_safe[k] = context[k]
+                        except TypeError as te:
+                            # hackity-hacky: we serialize the structure 
using a default serializer handler, then load
+                            # it from JSON so it can be re-serialized 
later in the proper context


# hackity-hacky !!

We're doing re-factoring to remove hacks and awkward code, let's not be 
adding them back in at the same rate.

Michael



On 19/05/15 16:27, Damian, Alexandru wrote:
> Hi,
>
> Can you please review this branch ? It brings in an important piece of 
> refactoring to move Toaster towards the REST-style API that will 
> enable a cleaner design and support for interconnection with other 
> systems.
>
> Getting into details, the "New build" button in all the pages, bar the 
> project page, now fetches data using the projects API, which is just 
> the "all projects" page in machine-readable format. This cleans up a 
> bit the overloading of the xhr_datatypeahead; after the API 
> refactoring, all xhr_ calls should be completely replaced with proper 
> calls to the REST endpoints.
>
> The code is here:
> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>
> Incidentally, this patch fixes the "new build" breakage by the first 
> round of URL refactoring, and the "construction" of URLs on the client 
> side.
>
> Can you please review and comment ?
>
> 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.
>

Re: [review-request] adamian/20150515_fix_table_header

[Barros Pena, Belen]

On 19/05/2015 16:27, "Damian, Alexandru" <alexandru.damian@intel.com>
wrote:

>Hi,
>
>
>Can you please review this branch ? It brings in an important piece of
>refactoring to move Toaster towards the REST-style API that will enable a
>cleaner design and support for interconnection
> with other systems.
>
>
>Getting into details, the "New build" button in all the pages, bar the
>project page, now fetches data using the projects API, which is just the
>"all projects" page in machine-readable format.

Hi Alex:

I've looked at the UI, and I've found some issues with the interaction of
the new build button. Here comes a list:

* When no project is selected, the recipe input field is disabled, but the
build button is enabled, which looks a bit strange. We would need to
disable the build button as well while no project is selected

* When no project is selected, the 'Cancel' link needs to be disabled.
Otherwise you end up in a weird state you cannot get out of.


* The project name autocomplete now works, but when I select a project,
the 'change' icon is missing, which means I am stuck: I cannot select a
different project. The change icon needs to show next to the project name,
except when the number of projects in the Toaster instance is one.

* The recipes autocomplete is not working for me, but maybe it's not
related to this branch.

* If I select a project, then refresh the page, the page has forgotten
about the project I just selected. It would be nice to remember the last
project I chose. 

Thanks!

Belén

> This cleans up a bit the overloading of the xhr_datatypeahead; after the
>API refactoring, all xhr_ calls should be completely replaced with proper
>calls to the REST endpoints.
>
>
>The code is here:
>https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/201
>50515_fix_table_header
>
>
>
>Incidentally, this patch fixes the "new build" breakage by the first
>round of URL refactoring, and the "construction" of URLs on the client
>side.
>
>
>Can you please review and comment ?
>
>Cheers,
>Alex
>
>
>-- 
>Alex Damian
>Yocto Project
>
>SSG / OTC 
>
>
>

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 9742 bytes --]

Hi,

I've pushed a new version of the patch for review, on the same branch.


All of Belen's remarks have been amended, except

"* If I select a project, then refresh the page, the page has forgotten
about the project I just selected. It would be nice to remember the last
project I chose.
​"​

This is because, except for "All Projects" and "All Builds" pages, we
already have a default Project - the project under which we currently are.
We need a design decision to see which option has more priority - the
previous selection of the user, or the project under which the page is
displayed.


I've added a couple of tests in Django for toastergui  (separate patches)
to validate that the /projects endpoint returns valid data, and exemplify
the usage.

​I have more remarks below,

Cheers,
Alex​


On Tue, May 19, 2015 at 5:31 PM, Michael Wood <michael.g.wood@intel.com>
wrote:

> Hi,
>
> Review below.
>
> -
> -    libtoaster.makeTypeahead(newBuildProjectInput, { type : "projects" },
> function(item){
> +    libtoaster.makeTypeahead(newBuildProjectInput,
> libtoaster.ctx.projectsUrl, { type : "projects", format : "json" },
> function(item){
>          /* successfully selected a project */
>          newBuildProjectSaveBtn.removeAttr("disabled");
>          selectedProject = item;
> +
> +
>      })
>
>
> I understand the having the xhrUrl as a new parameter, but it doesn't make
> any sense to me to have "type" and "format", no one should expect setting
> the type to json as an argument to some magic urls in the request would
> then return something to consume for an API. How are you supposed to know
> which urls support JSON responses?  Really wouldn't be happy with that.
>

​I removed the "type" parameter in this particular call, since it made no
sense - the type of the object is specified by the URL in REST fashion.

All the REST endpoints will support JSON responses, and it will be easily
done by decorating them with the "template_renderer" decorator. This will
be done in a subsequent patch.

I do not follow the remark about "magic" URLs. All URLs are documented​ and
follow-able in REST fashion:

/projects/  - list all projects, or create new project (by POST)
/project/ID/ - list project by id, or update project data (by POST)
etc...

the idea is that the URLs called without the *format* parameter will return
text/html, and the same URLs called with the *format=json* parameter will
return application/json, in a predictible fashion


>
> If you're wanting to avoid having a query for the projects html page and
> another copy of it for the API response why not use a defined Queryset? the
> django QuerySet managers are exactly for this.
>
>
​What I want to avoid is view code replication, at all levels. e.g. the
computation for compatible layer versions are done separately two times,
with nearly identical code - when computing suggestions for returning JSON
files, and when displaying project layers table. There is no reason for
this code duplication, and for different API entry points.

This isn't just about selecting objects from the database, but also about
processing commands from the user - I think we should not have dual views
for whenever we need to render HTML or JSON - we only need to be
preoccupied with computing the correct data, and let the presentation layer
- in this case the template_renderer decorator - worry about presenting the
data in a suitable format.


>
> +      /* 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;
>
>
> What happens if something else accesses those afterwards? If you select a
> project then close the popup you've potentially broken any other "user" of
> that context data.
> Ideally those properties are read-only.
>

​They were already modifiable by ​building URLs by appending specific IDs
to base_ URLs - in this sense, the change only makes evident that the URLs
are not static at all. Furthermore, I agree that we shouldn't build URLs by
hand in JS code, so getting the  URLs in complete form from the backend is
cleaner, IMHO.

​Since these URLs ​live in the context of the page, they always operate on
the current project set in the context, no matter the caller. I think this
is in accordance with the current design of the "New Build" button, which
sets a current Project in page context for follow-up actions to be taken on
it - it would be strange if any pieces of code would want to operate on
another project without reflecting this to the user.



>
>
> +class RedirectException(Exception):
> +    def __init__(self, view, g, mandatory_parameters, *args, **kwargs):
> +        super(RedirectException, self).__init__()
> +        self.view = view
> +        self.g = g
> +        self.mandatory_parameters = mandatory_parameters
> +        self.oargs  = args
> +        self.okwargs = kwargs
> +
> +    def get_redirect_response(self):
> +        return _redirect_parameters(self.view, self.g,
> self.mandatory_parameters, self.oargs, self.okwargs)
> +
>
> Using HTTP redirects are a real pain because they get cached by the
> browser and are very difficult to invalidate.
>

​The redirects are now made temporary.

I think it's nicer to provide the user with defaults for options they
haven't selected, and I know no better way to reflect that to the user but
by issueing a redirect.​



>
>
> -    def projects(request):
> -        template="projects.html"
> +    def template_renderer(template):
> +        def func_wrapper(view):
> +            def returned_wrapper(request, *args, **kwargs):
> +                try:
> +                    context = view(request, *args, **kwargs)
> +                except RedirectException as e:
> +                    return e.get_redirect_response()
> +
> +                if request.GET.get('format', None) == 'json':
> +                    # objects is a special keyword - it's a Page, but we
> need the actual objects here
> +                    # in XHR, the objects come in the "list" property
> +                    if "objects" in context:
> +                        context["list"] = context["objects"].object_list
> +                        del context["objects"]
> +
> +                    # convert separately the values that are
> JSON-serializable
> +                    json_safe = {}
> +                    for k in context:
> +                        try:
> +                            jsonfilter(context[k])
> +                            json_safe[k] = context[k]
> +                        except TypeError as te:
> +                            # hackity-hacky: we serialize the structure
> using a default serializer handler, then load
> +                            # it from JSON so it can be re-serialized
> later in the proper context
>
>
> # hackity-hacky !!
>
> We're doing re-factoring to remove hacks and awkward code, let's not be
> adding them back in at the same rate.
>

​I've changed this to a cleaner version with no hacks.
​

>
> Michael
>
>
>
>
> On 19/05/15 16:27, Damian, Alexandru wrote:
>
>> Hi,
>>
>> Can you please review this branch ? It brings in an important piece of
>> refactoring to move Toaster towards the REST-style API that will enable a
>> cleaner design and support for interconnection with other systems.
>>
>> Getting into details, the "New build" button in all the pages, bar the
>> project page, now fetches data using the projects API, which is just the
>> "all projects" page in machine-readable format. This cleans up a bit the
>> overloading of the xhr_datatypeahead; after the API refactoring, all xhr_
>> calls should be completely replaced with proper calls to the REST endpoints.
>>
>> The code is here:
>>
>> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>>
>> Incidentally, this patch fixes the "new build" breakage by the first
>> round of URL refactoring, and the "construction" of URLs on the client side.
>>
>> Can you please review and comment ?
>>
>> 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.
>>
>>
> ---------------------------------------------------------------------
> 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

[-- Attachment #2: Type: text/html, Size: 15830 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 10261 bytes --]

Taken for submission,

Cheers,
Alex

On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru <
alexandru.damian@intel.com> wrote:

> Hi,
>
> I've pushed a new version of the patch for review, on the same branch.
>
>
> All of Belen's remarks have been amended, except
>
> "* If I select a project, then refresh the page, the page has forgotten
> about the project I just selected. It would be nice to remember the last
> project I chose.
> ​"​
>
> This is because, except for "All Projects" and "All Builds" pages, we
> already have a default Project - the project under which we currently are.
> We need a design decision to see which option has more priority - the
> previous selection of the user, or the project under which the page is
> displayed.
>
>
> I've added a couple of tests in Django for toastergui  (separate patches)
> to validate that the /projects endpoint returns valid data, and exemplify
> the usage.
>
> ​I have more remarks below,
>
> Cheers,
> Alex​
>
>
> On Tue, May 19, 2015 at 5:31 PM, Michael Wood <michael.g.wood@intel.com>
> wrote:
>
>> Hi,
>>
>> Review below.
>>
>> -
>> -    libtoaster.makeTypeahead(newBuildProjectInput, { type : "projects"
>> }, function(item){
>> +    libtoaster.makeTypeahead(newBuildProjectInput,
>> libtoaster.ctx.projectsUrl, { type : "projects", format : "json" },
>> function(item){
>>          /* successfully selected a project */
>>          newBuildProjectSaveBtn.removeAttr("disabled");
>>          selectedProject = item;
>> +
>> +
>>      })
>>
>>
>> I understand the having the xhrUrl as a new parameter, but it doesn't
>> make any sense to me to have "type" and "format", no one should expect
>> setting the type to json as an argument to some magic urls in the request
>> would then return something to consume for an API. How are you supposed to
>> know which urls support JSON responses?  Really wouldn't be happy with that.
>>
>
> ​I removed the "type" parameter in this particular call, since it made no
> sense - the type of the object is specified by the URL in REST fashion.
>
> All the REST endpoints will support JSON responses, and it will be easily
> done by decorating them with the "template_renderer" decorator. This will
> be done in a subsequent patch.
>
> I do not follow the remark about "magic" URLs. All URLs are documented​
> and follow-able in REST fashion:
>
> /projects/  - list all projects, or create new project (by POST)
> /project/ID/ - list project by id, or update project data (by POST)
> etc...
>
> the idea is that the URLs called without the *format* parameter will
> return text/html, and the same URLs called with the *format=json* parameter
> will return application/json, in a predictible fashion
>
>
>>
>> If you're wanting to avoid having a query for the projects html page and
>> another copy of it for the API response why not use a defined Queryset? the
>> django QuerySet managers are exactly for this.
>>
>>
> ​What I want to avoid is view code replication, at all levels. e.g. the
> computation for compatible layer versions are done separately two times,
> with nearly identical code - when computing suggestions for returning JSON
> files, and when displaying project layers table. There is no reason for
> this code duplication, and for different API entry points.
>
> This isn't just about selecting objects from the database, but also about
> processing commands from the user - I think we should not have dual views
> for whenever we need to render HTML or JSON - we only need to be
> preoccupied with computing the correct data, and let the presentation layer
> - in this case the template_renderer decorator - worry about presenting the
> data in a suitable format.
>
>
>>
>> +      /* 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;
>>
>>
>> What happens if something else accesses those afterwards? If you select a
>> project then close the popup you've potentially broken any other "user" of
>> that context data.
>> Ideally those properties are read-only.
>>
>
> ​They were already modifiable by ​building URLs by appending specific IDs
> to base_ URLs - in this sense, the change only makes evident that the URLs
> are not static at all. Furthermore, I agree that we shouldn't build URLs by
> hand in JS code, so getting the  URLs in complete form from the backend is
> cleaner, IMHO.
>
> ​Since these URLs ​live in the context of the page, they always operate on
> the current project set in the context, no matter the caller. I think this
> is in accordance with the current design of the "New Build" button, which
> sets a current Project in page context for follow-up actions to be taken on
> it - it would be strange if any pieces of code would want to operate on
> another project without reflecting this to the user.
>
>
>
>>
>>
>> +class RedirectException(Exception):
>> +    def __init__(self, view, g, mandatory_parameters, *args, **kwargs):
>> +        super(RedirectException, self).__init__()
>> +        self.view = view
>> +        self.g = g
>> +        self.mandatory_parameters = mandatory_parameters
>> +        self.oargs  = args
>> +        self.okwargs = kwargs
>> +
>> +    def get_redirect_response(self):
>> +        return _redirect_parameters(self.view, self.g,
>> self.mandatory_parameters, self.oargs, self.okwargs)
>> +
>>
>> Using HTTP redirects are a real pain because they get cached by the
>> browser and are very difficult to invalidate.
>>
>
> ​The redirects are now made temporary.
>
> I think it's nicer to provide the user with defaults for options they
> haven't selected, and I know no better way to reflect that to the user but
> by issueing a redirect.​
>
>
>
>>
>>
>> -    def projects(request):
>> -        template="projects.html"
>> +    def template_renderer(template):
>> +        def func_wrapper(view):
>> +            def returned_wrapper(request, *args, **kwargs):
>> +                try:
>> +                    context = view(request, *args, **kwargs)
>> +                except RedirectException as e:
>> +                    return e.get_redirect_response()
>> +
>> +                if request.GET.get('format', None) == 'json':
>> +                    # objects is a special keyword - it's a Page, but we
>> need the actual objects here
>> +                    # in XHR, the objects come in the "list" property
>> +                    if "objects" in context:
>> +                        context["list"] = context["objects"].object_list
>> +                        del context["objects"]
>> +
>> +                    # convert separately the values that are
>> JSON-serializable
>> +                    json_safe = {}
>> +                    for k in context:
>> +                        try:
>> +                            jsonfilter(context[k])
>> +                            json_safe[k] = context[k]
>> +                        except TypeError as te:
>> +                            # hackity-hacky: we serialize the structure
>> using a default serializer handler, then load
>> +                            # it from JSON so it can be re-serialized
>> later in the proper context
>>
>>
>> # hackity-hacky !!
>>
>> We're doing re-factoring to remove hacks and awkward code, let's not be
>> adding them back in at the same rate.
>>
>
> ​I've changed this to a cleaner version with no hacks.
> ​
>
>>
>> Michael
>>
>>
>>
>>
>> On 19/05/15 16:27, Damian, Alexandru wrote:
>>
>>> Hi,
>>>
>>> Can you please review this branch ? It brings in an important piece of
>>> refactoring to move Toaster towards the REST-style API that will enable a
>>> cleaner design and support for interconnection with other systems.
>>>
>>> Getting into details, the "New build" button in all the pages, bar the
>>> project page, now fetches data using the projects API, which is just the
>>> "all projects" page in machine-readable format. This cleans up a bit the
>>> overloading of the xhr_datatypeahead; after the API refactoring, all xhr_
>>> calls should be completely replaced with proper calls to the REST endpoints.
>>>
>>> The code is here:
>>>
>>> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>>>
>>> Incidentally, this patch fixes the "new build" breakage by the first
>>> round of URL refactoring, and the "construction" of URLs on the client side.
>>>
>>> Can you please review and comment ?
>>>
>>> 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.
>>>
>>>
>> ---------------------------------------------------------------------
>> 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

[-- Attachment #2: Type: text/html, Size: 16791 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Barros Pena, Belen]

On 21/05/2015 11:12, "Damian, Alexandru" <alexandru.damian@intel.com>
wrote:

>Taken for submission,
>
>
>Cheers,
>
>Alex
>
>
>
>On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru
><alexandru.damian@intel.com> wrote:
>
>Hi,
>
>
>I've pushed a new version of the patch for review, on the same branch.
>
>
>
>
>All of Belen's remarks have been amended, except

The 'change' icon is showing now when there is only one project in the
Toaster instance :/

Cheers

Belén


> 
>
>
>"* If I select a project, then refresh the page, the page has forgotten
>about the project I just selected. It would be nice to remember the last
>project I chose.
>​"​
>
>
>
>This is because, except for "All Projects" and "All Builds" pages, we
>already have a default Project - the project under which we currently
>are. We need a design decision to see
> which option has more priority - the previous selection of the user, or
>the project under which the page is displayed.
>
>
>
>
>
>
>
>I've added a couple of tests in Django for toastergui  (separate patches)
>to validate that the /projects endpoint returns valid data, and exemplify
>the usage.
>
>
>
>
>​I have more remarks below,
>
>Cheers,
>Alex​
>
>
>
>On Tue, May 19, 2015 at 5:31 PM, Michael Wood
><michael.g.wood@intel.com> wrote:
>
>Hi,
>
>Review below.
>
>-
>-    libtoaster.makeTypeahead(newBuildProjectInput, { type : "projects"
>}, function(item){
>+    libtoaster.makeTypeahead(newBuildProjectInput,
>libtoaster.ctx.projectsUrl, { type : "projects", format : "json" },
>function(item){
>         /* successfully selected a project */
>         newBuildProjectSaveBtn.removeAttr("disabled");
>         selectedProject = item;
>+
>+
>     })
>
>
>I understand the having the xhrUrl as a new parameter, but it doesn't
>make any sense to me to have "type" and "format", no one should expect
>setting the type to json as an argument to some magic urls in the request
>would then return something to consume for
> an API. How are you supposed to know which urls support JSON responses?
>Really wouldn't be happy with that.
>
>
>
>
>​I removed the "type" parameter in this particular call, since it made no
>sense - the type of the object is specified by the URL in REST fashion.
>
>
>All the REST endpoints will support JSON responses, and it will be easily
>done by decorating them with the "template_renderer" decorator. This will
>be done in a subsequent patch.
>
>
>I do not follow the remark about "magic" URLs. All URLs are documented​
>and follow-able in REST fashion:
>
>
>
>/projects/  - list all projects, or create new project (by POST)
>/project/ID/ - list project by id, or update project data (by POST)
>etc...
>
>
>the idea is that the URLs called without the
>format parameter will return text/html, and the same URLs called with the
>format=json parameter will return application/json, in a predictible
>fashion
> 
>
>
>If you're wanting to avoid having a query for the projects html page and
>another copy of it for the API response why not use a defined Queryset?
>the django QuerySet managers are exactly for this.
>
>
>
>
>
>​What I want to avoid is view code replication, at all levels. e.g. the
>computation for compatible layer versions are done separately two times,
>with nearly identical code - when computing suggestions
> for returning JSON files, and when displaying project layers table.
>There is no reason for this code duplication, and for different API entry
>points.
>
>
>This isn't just about selecting objects from the database, but also about
>processing commands from the user - I think we should not have dual views
>for whenever we need to render HTML or JSON
> - we only need to be preoccupied with computing the correct data, and
>let the presentation layer - in this case the template_renderer decorator
>- worry about presenting the data in a suitable format.
>
>
>
>
>
>
>+      /* 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;
>
>
>What happens if something else accesses those afterwards? If you select a
>project then close the popup you've potentially broken any other "user"
>of that context data.
>Ideally those properties are read-only.
>
>
>
>
>​They were already modifiable by ​building URLs by appending specific IDs
>to base_ URLs - in this sense, the change only makes evident that the
>URLs are not static at all. Furthermore, I agree
> that we shouldn't build URLs by hand in JS code, so getting the  URLs in
>complete form from the backend is cleaner, IMHO.
>
>
>​Since these URLs ​live in the context of the page, they always operate
>on the current project set in the context, no matter the caller. I think
>this is in accordance with the current design of
> the "New Build" button, which sets a current Project in page context for
>follow-up actions to be taken on it - it would be strange if any pieces
>of code would want to operate on another project without reflecting this
>to the user.
>
>
> 
>
>
>
>+class RedirectException(Exception):
>+    def __init__(self, view, g, mandatory_parameters, *args, **kwargs):
>+        super(RedirectException, self).__init__()
>+        self.view = view
>+        self.g = g
>+        self.mandatory_parameters = mandatory_parameters
>+        self.oargs  = args
>+        self.okwargs = kwargs
>+
>+    def get_redirect_response(self):
>+        return _redirect_parameters(self.view, self.g,
>self.mandatory_parameters, self.oargs, self.okwargs)
>+
>
>Using HTTP redirects are a real pain because they get cached by the
>browser and are very difficult to invalidate.
>
>
>
>
>​The redirects are now made temporary.
>
>
>I think it's nicer to provide the user with defaults for options they
>haven't selected, and I know no better way to reflect that to the user
>but by issueing a redirect.​
>
>
> 
>
>
>
>-    def projects(request):
>-        template="projects.html"
>+    def template_renderer(template):
>+        def func_wrapper(view):
>+            def returned_wrapper(request, *args, **kwargs):
>+                try:
>+                    context = view(request, *args, **kwargs)
>+                except RedirectException as e:
>+                    return e.get_redirect_response()
>+
>+                if request.GET.get('format', None) == 'json':
>+                    # objects is a special keyword - it's a Page, but we
>need the actual objects here
>+                    # in XHR, the objects come in the "list" property
>+                    if "objects" in context:
>+                        context["list"] = context["objects"].object_list
>+                        del context["objects"]
>+
>+                    # convert separately the values that are
>JSON-serializable
>+                    json_safe = {}
>+                    for k in context:
>+                        try:
>+                            jsonfilter(context[k])
>+                            json_safe[k] = context[k]
>+                        except TypeError as te:
>+                            # hackity-hacky: we serialize the structure
>using a default serializer handler, then load
>+                            # it from JSON so it can be re-serialized
>later in the proper context
>
>
># hackity-hacky !!
>
>We're doing re-factoring to remove hacks and awkward code, let's not be
>adding them back in at the same rate.
>
>
>
>
>
>
>​I've changed this to a cleaner version with no hacks.
>​
>
>
>
>Michael
>
>
>
>
>On 19/05/15 16:27, Damian, Alexandru wrote:
>
>
>Hi,
>
>Can you please review this branch ? It brings in an important piece of
>refactoring to move Toaster towards the REST-style API that will enable a
>cleaner design and support for interconnection with other systems.
>
>Getting into details, the "New build" button in all the pages, bar the
>project page, now fetches data using the projects API, which is just the
>"all projects" page in machine-readable format. This cleans up a bit the
>overloading of the xhr_datatypeahead; after
> the API refactoring, all xhr_ calls should be completely replaced with
>proper calls to the REST endpoints.
>
>The code is here:
>https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/201
>50515_fix_table_header
>
>Incidentally, this patch fixes the "new build" breakage by the first
>round of URL refactoring, and the "construction" of URLs on the client
>side.
>
>Can you please review and comment ?
>
>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.
>
>
>
>
>---------------------------------------------------------------------
>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 
>
>
>

Re: [review-request] adamian/20150515_fix_table_header

[Michael Wood]

Hi,

Conflating the web pages and the APIs into a single URL pattern/schema 
just doesn't make sense to me because:

- We will have pages calling themselves with a different parameter (e.g. 
the tables pages)
- This is not how REST frameworks are implemented in any other 
application I've seen before
- In the future we may want to version the name-space e.g. 
/api/1.3/projects/
- Keeping the API self contained allows for greater future flexibility 
because it de-couples them from the page structure.
- REST should be as self documenting as possible. Having to know which 
URLs support JSON responses isn't helpful.
- The tree of objects in a web page doesn't not have a 1:1 mapping with 
the API so they functionally different, in only a few cases do we have 1 
page = 1 set of data.
- The duplication is just having a url entry and a view that has a 
query+json response (you could easily do this as subclassed view, 
ToasterJsonView.as_view(QuerySet)) really that's not a lot. If you use 
pre-defined querysets that's even easier, the abstraction can make the 
duplication negligible.
- Soon the API calls are going to be the main way in which toaster get's 
it's data and the template mechanism is going to be used very lightly.

There are still a few other issues in the patch, but I think we need to 
get this sorted first as it affects everything else.

I'd be much happier to have along these lines

/api/projects/
/api/project/1/
/api/project/1/?bitbake="core-image-minimal"
.
.

Current ToasterTable handles this already (could be tweaked to have a 
different modes):

/api/project/1/compatible/machines/
/api/project/1/compatible/layers/
/api/project/1/compatible/recipes/
.
.

To add (though we currently only have a use case for layer at the moment):

/api/project/1/compatible/layer/3
/api/project/1/compatible/machine/3
/api/project/1/compatible/recipe/3
.
.


Michael



On 21/05/15 11:12, Damian, Alexandru wrote:
> Taken for submission,
>
> Cheers,
> Alex
>
> On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru 
> <alexandru.damian@intel.com <mailto:alexandru.damian@intel.com>> wrote:
>
>     Hi,
>
>     I've pushed a new version of the patch for review, on the same branch.
>
>
>     All of Belen's remarks have been amended, except
>
>     "* If I select a project, then refresh the page, the page has
>     forgotten
>     about the project I just selected. It would be nice to remember
>     the last
>     project I chose.
>     ​ "​
>
>     This is because, except for "All Projects" and "All Builds" pages,
>     we already have a default Project - the project under which we
>     currently are. We need a design decision to see which option has
>     more priority - the previous selection of the user, or the project
>     under which the page is displayed.
>
>
>     I've added a couple of tests in Django for toastergui  (separate
>     patches) to validate that the /projects endpoint returns valid
>     data, and exemplify the usage.
>
>     ​I have more remarks below,
>
>     Cheers,
>     Alex​
>
>
>     On Tue, May 19, 2015 at 5:31 PM, Michael Wood
>     <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>> wrote:
>
>         Hi,
>
>         Review below.
>
>         -
>         - libtoaster.makeTypeahead(newBuildProjectInput, { type :
>         "projects" }, function(item){
>         + libtoaster.makeTypeahead(newBuildProjectInput,
>         libtoaster.ctx.projectsUrl, { type : "projects", format :
>         "json" }, function(item){
>                  /* successfully selected a project */
>          newBuildProjectSaveBtn.removeAttr("disabled");
>                  selectedProject = item;
>         +
>         +
>              })
>
>
>         I understand the having the xhrUrl as a new parameter, but it
>         doesn't make any sense to me to have "type" and "format", no
>         one should expect setting the type to json as an argument to
>         some magic urls in the request would then return something to
>         consume for an API. How are you supposed to know which urls
>         support JSON responses?  Really wouldn't be happy with that.
>
>
>     ​I removed the "type" parameter in this particular call, since it
>     made no sense - the type of the object is specified by the URL in
>     REST fashion.
>
>     All the REST endpoints will support JSON responses, and it will be
>     easily done by decorating them with the "template_renderer"
>     decorator. This will be done in a subsequent patch.
>
>     I do not follow the remark about "magic" URLs. All URLs are
>     documented​ and follow-able in REST fashion:
>
>     /projects/  - list all projects, or create new project (by POST)
>     /project/ID/ - list project by id, or update project data (by POST)
>     etc...
>
>     the idea is that the URLs called without the *format* parameter
>     will return text/html, and the same URLs called with the
>     *format=json* parameter will return application/json, in a
>     predictible fashion
>
>
>         If you're wanting to avoid having a query for the projects
>         html page and another copy of it for the API response why not
>         use a defined Queryset? the django QuerySet managers are
>         exactly for this.
>
>
>     ​What I want to avoid is view code replication, at all levels.
>     e.g. the computation for compatible layer versions are done
>     separately two times, with nearly identical code - when computing
>     suggestions for returning JSON files, and when displaying project
>     layers table. There is no reason for this code duplication, and
>     for different API entry points.
>
>     This isn't just about selecting objects from the database, but
>     also about processing commands from the user - I think we should
>     not have dual views for whenever we need to render HTML or JSON -
>     we only need to be preoccupied with computing the correct data,
>     and let the presentation layer - in this case the
>     template_renderer decorator - worry about presenting the data in a
>     suitable format.
>
>
>
>         +      /* 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;
>
>
>         What happens if something else accesses those afterwards? If
>         you select a project then close the popup you've potentially
>         broken any other "user" of that context data.
>         Ideally those properties are read-only.
>
>
>     ​They were already modifiable by ​building URLs by appending
>     specific IDs to base_ URLs - in this sense, the change only makes
>     evident that the URLs are not static at all. Furthermore, I agree
>     that we shouldn't build URLs by hand in JS code, so getting the
>      URLs in complete form from the backend is cleaner, IMHO.
>
>     ​Since these URLs ​live in the context of the page, they always
>     operate on the current project set in the context, no matter the
>     caller. I think this is in accordance with the current design of
>     the "New Build" button, which sets a current Project in page
>     context for follow-up actions to be taken on it - it would be
>     strange if any pieces of code would want to operate on another
>     project without reflecting this to the user.
>
>
>
>         +class RedirectException(Exception):
>         +    def __init__(self, view, g, mandatory_parameters, *args,
>         **kwargs):
>         +        super(RedirectException, self).__init__()
>         +        self.view = view
>         +        self.g = g
>         +        self.mandatory_parameters = mandatory_parameters
>         +        self.oargs  = args
>         +        self.okwargs = kwargs
>         +
>         +    def get_redirect_response(self):
>         +        return _redirect_parameters(self.view, self.g,
>         self.mandatory_parameters, self.oargs, self.okwargs)
>         +
>
>         Using HTTP redirects are a real pain because they get cached
>         by the browser and are very difficult to invalidate.
>
>
>     ​The redirects are now made temporary.
>
>     I think it's nicer to provide the user with defaults for options
>     they haven't selected, and I know no better way to reflect that to
>     the user but by issueing a redirect.​
>
>
>
>         -    def projects(request):
>         -        template="projects.html"
>         +    def template_renderer(template):
>         +        def func_wrapper(view):
>         +            def returned_wrapper(request, *args, **kwargs):
>         +                try:
>         +                    context = view(request, *args, **kwargs)
>         +                except RedirectException as e:
>         +                    return e.get_redirect_response()
>         +
>         +                if request.GET.get('format', None) == 'json':
>         +                    # objects is a special keyword - it's a
>         Page, but we need the actual objects here
>         +                    # in XHR, the objects come in the "list"
>         property
>         +                    if "objects" in context:
>         +                        context["list"] =
>         context["objects"].object_list
>         +                        del context["objects"]
>         +
>         +                    # convert separately the values that are
>         JSON-serializable
>         +                    json_safe = {}
>         +                    for k in context:
>         +                        try:
>         + jsonfilter(context[k])
>         +                            json_safe[k] = context[k]
>         +                        except TypeError as te:
>         +                            # hackity-hacky: we serialize the
>         structure using a default serializer handler, then load
>         +                            # it from JSON so it can be
>         re-serialized later in the proper context
>
>
>         # hackity-hacky !!
>
>         We're doing re-factoring to remove hacks and awkward code,
>         let's not be adding them back in at the same rate.
>
>
>     ​I've changed this to a cleaner version with no hacks.
>     ​
>
>
>         Michael
>
>
>
>
>         On 19/05/15 16:27, Damian, Alexandru wrote:
>
>             Hi,
>
>             Can you please review this branch ? It brings in an
>             important piece of refactoring to move Toaster towards the
>             REST-style API that will enable a cleaner design and
>             support for interconnection with other systems.
>
>             Getting into details, the "New build" button in all the
>             pages, bar the project page, now fetches data using the
>             projects API, which is just the "all projects" page in
>             machine-readable format. This cleans up a bit the
>             overloading of the xhr_datatypeahead; after the API
>             refactoring, all xhr_ calls should be completely replaced
>             with proper calls to the REST endpoints.
>
>             The code is here:
>             https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>
>             Incidentally, this patch fixes the "new build" breakage by
>             the first round of URL refactoring, and the "construction"
>             of URLs on the client side.
>
>             Can you please review and comment ?
>
>             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.
>
>
>         ---------------------------------------------------------------------
>         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
>
> ---------------------------------------------------------------------
> 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.
>

Re: [review-request] adamian/20150515_fix_table_header

[Barros Pena, Belen]

On 21/05/2015 11:23, "Barros Pena, Belen" <belen.barros.pena@intel.com>
wrote:

>
>This is because, except for "All Projects" and "All Builds" pages, we
>already have a default Project - the project under which we currently
>are. We need a design decision to see
>which option has more priority - the previous selection of the user, or
>the project under which the page is displayed.

Well, in an ideal world, if you are in a page under a project, the current
project takes priority.

If you are in a page that is not under a project (i.e. All builds or All
projects), the latest project selected by the user, or the project of the
latest build in Toaster would also do.

Cheers

Belén

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 15277 bytes --]

Reverting the last patch from submission in order to further discuss it

Alex

On Thu, May 21, 2015 at 11:25 AM, Michael Wood <michael.g.wood@intel.com>
wrote:

> Hi,
>
> Conflating the web pages and the APIs into a single URL pattern/schema
> just doesn't make sense to me because:
>
> - We will have pages calling themselves with a different parameter (e.g.
> the tables pages)
> - This is not how REST frameworks are implemented in any other application
> I've seen before
> - In the future we may want to version the name-space e.g.
> /api/1.3/projects/
> - Keeping the API self contained allows for greater future flexibility
> because it de-couples them from the page structure.
> - REST should be as self documenting as possible. Having to know which
> URLs support JSON responses isn't helpful.
> - The tree of objects in a web page doesn't not have a 1:1 mapping with
> the API so they functionally different, in only a few cases do we have 1
> page = 1 set of data.
> - The duplication is just having a url entry and a view that has a
> query+json response (you could easily do this as subclassed view,
> ToasterJsonView.as_view(QuerySet)) really that's not a lot. If you use
> pre-defined querysets that's even easier, the abstraction can make the
> duplication negligible.
> - Soon the API calls are going to be the main way in which toaster get's
> it's data and the template mechanism is going to be used very lightly.
>
> There are still a few other issues in the patch, but I think we need to
> get this sorted first as it affects everything else.
>
> I'd be much happier to have along these lines
>
> /api/projects/
> /api/project/1/
> /api/project/1/?bitbake="core-image-minimal"
> .
> .
>
> Current ToasterTable handles this already (could be tweaked to have a
> different modes):
>
> /api/project/1/compatible/machines/
> /api/project/1/compatible/layers/
> /api/project/1/compatible/recipes/
> .
> .
>
> To add (though we currently only have a use case for layer at the moment):
>
> /api/project/1/compatible/layer/3
> /api/project/1/compatible/machine/3
> /api/project/1/compatible/recipe/3
> .
> .
>
>
> Michael
>
>
>
> On 21/05/15 11:12, Damian, Alexandru wrote:
>
>> Taken for submission,
>>
>> Cheers,
>> Alex
>>
>> On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru <
>> alexandru.damian@intel.com <mailto:alexandru.damian@intel.com>> wrote:
>>
>>     Hi,
>>
>>     I've pushed a new version of the patch for review, on the same branch.
>>
>>
>>     All of Belen's remarks have been amended, except
>>
>>     "* If I select a project, then refresh the page, the page has
>>     forgotten
>>     about the project I just selected. It would be nice to remember
>>     the last
>>     project I chose.
>>     ​ "​
>>
>>     This is because, except for "All Projects" and "All Builds" pages,
>>     we already have a default Project - the project under which we
>>     currently are. We need a design decision to see which option has
>>     more priority - the previous selection of the user, or the project
>>     under which the page is displayed.
>>
>>
>>     I've added a couple of tests in Django for toastergui  (separate
>>     patches) to validate that the /projects endpoint returns valid
>>     data, and exemplify the usage.
>>
>>     ​I have more remarks below,
>>
>>     Cheers,
>>     Alex​
>>
>>
>>     On Tue, May 19, 2015 at 5:31 PM, Michael Wood
>>     <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>> wrote:
>>
>>         Hi,
>>
>>         Review below.
>>
>>         -
>>         - libtoaster.makeTypeahead(newBuildProjectInput, { type :
>>         "projects" }, function(item){
>>         + libtoaster.makeTypeahead(newBuildProjectInput,
>>         libtoaster.ctx.projectsUrl, { type : "projects", format :
>>         "json" }, function(item){
>>                  /* successfully selected a project */
>>          newBuildProjectSaveBtn.removeAttr("disabled");
>>                  selectedProject = item;
>>         +
>>         +
>>              })
>>
>>
>>         I understand the having the xhrUrl as a new parameter, but it
>>         doesn't make any sense to me to have "type" and "format", no
>>         one should expect setting the type to json as an argument to
>>         some magic urls in the request would then return something to
>>         consume for an API. How are you supposed to know which urls
>>         support JSON responses?  Really wouldn't be happy with that.
>>
>>
>>     ​I removed the "type" parameter in this particular call, since it
>>     made no sense - the type of the object is specified by the URL in
>>     REST fashion.
>>
>>     All the REST endpoints will support JSON responses, and it will be
>>     easily done by decorating them with the "template_renderer"
>>     decorator. This will be done in a subsequent patch.
>>
>>     I do not follow the remark about "magic" URLs. All URLs are
>>     documented​ and follow-able in REST fashion:
>>
>>     /projects/  - list all projects, or create new project (by POST)
>>     /project/ID/ - list project by id, or update project data (by POST)
>>     etc...
>>
>>     the idea is that the URLs called without the *format* parameter
>>     will return text/html, and the same URLs called with the
>>     *format=json* parameter will return application/json, in a
>>
>>     predictible fashion
>>
>>
>>         If you're wanting to avoid having a query for the projects
>>         html page and another copy of it for the API response why not
>>         use a defined Queryset? the django QuerySet managers are
>>         exactly for this.
>>
>>
>>     ​What I want to avoid is view code replication, at all levels.
>>     e.g. the computation for compatible layer versions are done
>>     separately two times, with nearly identical code - when computing
>>     suggestions for returning JSON files, and when displaying project
>>     layers table. There is no reason for this code duplication, and
>>     for different API entry points.
>>
>>     This isn't just about selecting objects from the database, but
>>     also about processing commands from the user - I think we should
>>     not have dual views for whenever we need to render HTML or JSON -
>>     we only need to be preoccupied with computing the correct data,
>>     and let the presentation layer - in this case the
>>     template_renderer decorator - worry about presenting the data in a
>>     suitable format.
>>
>>
>>
>>         +      /* 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;
>>
>>
>>         What happens if something else accesses those afterwards? If
>>         you select a project then close the popup you've potentially
>>         broken any other "user" of that context data.
>>         Ideally those properties are read-only.
>>
>>
>>     ​They were already modifiable by ​building URLs by appending
>>     specific IDs to base_ URLs - in this sense, the change only makes
>>     evident that the URLs are not static at all. Furthermore, I agree
>>     that we shouldn't build URLs by hand in JS code, so getting the
>>      URLs in complete form from the backend is cleaner, IMHO.
>>
>>     ​Since these URLs ​live in the context of the page, they always
>>     operate on the current project set in the context, no matter the
>>     caller. I think this is in accordance with the current design of
>>     the "New Build" button, which sets a current Project in page
>>     context for follow-up actions to be taken on it - it would be
>>     strange if any pieces of code would want to operate on another
>>     project without reflecting this to the user.
>>
>>
>>
>>         +class RedirectException(Exception):
>>         +    def __init__(self, view, g, mandatory_parameters, *args,
>>         **kwargs):
>>         +        super(RedirectException, self).__init__()
>>         +        self.view = view
>>         +        self.g = g
>>         +        self.mandatory_parameters = mandatory_parameters
>>         +        self.oargs  = args
>>         +        self.okwargs = kwargs
>>         +
>>         +    def get_redirect_response(self):
>>         +        return _redirect_parameters(self.view, self.g,
>>         self.mandatory_parameters, self.oargs, self.okwargs)
>>         +
>>
>>         Using HTTP redirects are a real pain because they get cached
>>         by the browser and are very difficult to invalidate.
>>
>>
>>     ​The redirects are now made temporary.
>>
>>     I think it's nicer to provide the user with defaults for options
>>     they haven't selected, and I know no better way to reflect that to
>>     the user but by issueing a redirect.​
>>
>>
>>
>>         -    def projects(request):
>>         -        template="projects.html"
>>         +    def template_renderer(template):
>>         +        def func_wrapper(view):
>>         +            def returned_wrapper(request, *args, **kwargs):
>>         +                try:
>>         +                    context = view(request, *args, **kwargs)
>>         +                except RedirectException as e:
>>         +                    return e.get_redirect_response()
>>         +
>>         +                if request.GET.get('format', None) == 'json':
>>         +                    # objects is a special keyword - it's a
>>         Page, but we need the actual objects here
>>         +                    # in XHR, the objects come in the "list"
>>         property
>>         +                    if "objects" in context:
>>         +                        context["list"] =
>>         context["objects"].object_list
>>         +                        del context["objects"]
>>         +
>>         +                    # convert separately the values that are
>>         JSON-serializable
>>         +                    json_safe = {}
>>         +                    for k in context:
>>         +                        try:
>>         + jsonfilter(context[k])
>>         +                            json_safe[k] = context[k]
>>         +                        except TypeError as te:
>>         +                            # hackity-hacky: we serialize the
>>         structure using a default serializer handler, then load
>>         +                            # it from JSON so it can be
>>         re-serialized later in the proper context
>>
>>
>>         # hackity-hacky !!
>>
>>         We're doing re-factoring to remove hacks and awkward code,
>>         let's not be adding them back in at the same rate.
>>
>>
>>     ​I've changed this to a cleaner version with no hacks.
>>     ​
>>
>>
>>         Michael
>>
>>
>>
>>
>>         On 19/05/15 16:27, Damian, Alexandru wrote:
>>
>>             Hi,
>>
>>             Can you please review this branch ? It brings in an
>>             important piece of refactoring to move Toaster towards the
>>             REST-style API that will enable a cleaner design and
>>             support for interconnection with other systems.
>>
>>             Getting into details, the "New build" button in all the
>>             pages, bar the project page, now fetches data using the
>>             projects API, which is just the "all projects" page in
>>             machine-readable format. This cleans up a bit the
>>             overloading of the xhr_datatypeahead; after the API
>>             refactoring, all xhr_ calls should be completely replaced
>>             with proper calls to the REST endpoints.
>>
>>             The code is here:
>>
>> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>>
>>             Incidentally, this patch fixes the "new build" breakage by
>>             the first round of URL refactoring, and the "construction"
>>             of URLs on the client side.
>>
>>             Can you please review and comment ?
>>
>>             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.
>>
>>
>>
>> ---------------------------------------------------------------------
>>         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
>>
>> ---------------------------------------------------------------------
>> 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.
>>
>>
> ---------------------------------------------------------------------
> 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

[-- Attachment #2: Type: text/html, Size: 18441 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 17024 bytes --]

Hello,

I have several comments below - I hope this will shed light on this
approach.

Cheers,
Alex

On Thu, May 21, 2015 at 11:25 AM, Michael Wood <michael.g.wood@intel.com>
wrote:

> Hi,
>
> Conflating the web pages and the APIs into a single URL pattern/schema
> just doesn't make sense to me because:
>
> - We will have pages calling themselves with a different parameter (e.g.
> the tables pages)
>

​And this is quite ok, since it will return the same data, but in a
different format. The whole idea of REST is to allow a unique point of
entry for each resource - so the table data in HTML format and in JSON
format will be at the same URI.​



> - This is not how REST frameworks are implemented in any other application
> I've seen before
>

​I've taken the browsable-api from Django REST framework as model; which
has the same concept of exposing data in different formats based on a GET
parameter

http://www.django-rest-framework.org/topics/browsable-api/​



> - In the future we may want to version the name-space e.g.
> /api/1.3/projects/
>

And this approach will make it easier - we will only have to port a single
set of URLs ​and not pairs for JSON and HTML content.



> - Keeping the API self contained allows for greater future flexibility
> because it de-couples them from the page structure.
>

​Exactly my point - the HTML page structure is created in templates, while
the data itself is built in the view context; this approach enforces actual
encapsulation of data in the context, a issue we confronted in the past.


> - REST should be as self documenting as possible. Having to know which
> URLs support JSON responses isn't helpful.
>

​All REST endpoints (indeed, all URLs) will support JSON formats.​


> - The tree of objects in a web page doesn't not have a 1:1 mapping with
> the API so they functionally different, in only a few cases do we have 1
> page = 1 set of data.
>

​Correct, the mapping is view context <-> actual data. Extra data coming in
from the template should be generated from context processors, if it's not
in the primary set of data that's interesting to users.​


> - The duplication is just having a url entry and a view that has a
> query+json response (you could easily do this as subclassed view,
> ToasterJsonView.as_view(QuerySet)) really that's not a lot. If you use
> pre-defined querysets that's even easier, the abstraction can make the
> duplication negligible.
>

​I think this duplication is something that we should've avoided from the
start. After discussing this with Vlad, the dynamic URL construction
mechanism for XHR APIs is not very intuitive to the user; also, it breaks
the REST paradigm.​



> - Soon the API calls are going to be the main way in which toaster get's
> it's data and the template mechanism is going to be used very lightly.
>
> There are still a few other issues in the patch, but I think we need to
> get this sorted first as it affects everything else.
>
> I'd be much happier to have along these lines
>
> /api/projects/
> /api/project/1/
> /api/project/1/?bitbake="core-image-minimal"
> .
> .
>
> Current ToasterTable handles this already (could be tweaked to have a
> different modes):
>
> /api/project/1/compatible/machines/
> /api/project/1/compatible/layers/
> /api/project/1/compatible/recipes/
> .
>

​I already expressed why I think accepting this approach was not ideal as
it breaks the REST principles - having separate URIs for different formats
is hardly ideal. Also, the current mechanism in ToasterTables to generate
the dynamic URIs is not ideal either.



> .
>
> To add (though we currently only have a use case for layer at the moment):
>
> /api/project/1/compatible/layer/3
> /api/project/1/compatible/machine/3
> /api/project/1/compatible/recipe/3
> .
> .
>
>
> Michael
>
>
>
> On 21/05/15 11:12, Damian, Alexandru wrote:
>
>> Taken for submission,
>>
>> Cheers,
>> Alex
>>
>> On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru <
>> alexandru.damian@intel.com <mailto:alexandru.damian@intel.com>> wrote:
>>
>>     Hi,
>>
>>     I've pushed a new version of the patch for review, on the same branch.
>>
>>
>>     All of Belen's remarks have been amended, except
>>
>>     "* If I select a project, then refresh the page, the page has
>>     forgotten
>>     about the project I just selected. It would be nice to remember
>>     the last
>>     project I chose.
>>     ​ "​
>>
>>     This is because, except for "All Projects" and "All Builds" pages,
>>     we already have a default Project - the project under which we
>>     currently are. We need a design decision to see which option has
>>     more priority - the previous selection of the user, or the project
>>     under which the page is displayed.
>>
>>
>>     I've added a couple of tests in Django for toastergui  (separate
>>     patches) to validate that the /projects endpoint returns valid
>>     data, and exemplify the usage.
>>
>>     ​I have more remarks below,
>>
>>     Cheers,
>>     Alex​
>>
>>
>>     On Tue, May 19, 2015 at 5:31 PM, Michael Wood
>>     <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>> wrote:
>>
>>         Hi,
>>
>>         Review below.
>>
>>         -
>>         - libtoaster.makeTypeahead(newBuildProjectInput, { type :
>>         "projects" }, function(item){
>>         + libtoaster.makeTypeahead(newBuildProjectInput,
>>         libtoaster.ctx.projectsUrl, { type : "projects", format :
>>         "json" }, function(item){
>>                  /* successfully selected a project */
>>          newBuildProjectSaveBtn.removeAttr("disabled");
>>                  selectedProject = item;
>>         +
>>         +
>>              })
>>
>>
>>         I understand the having the xhrUrl as a new parameter, but it
>>         doesn't make any sense to me to have "type" and "format", no
>>         one should expect setting the type to json as an argument to
>>         some magic urls in the request would then return something to
>>         consume for an API. How are you supposed to know which urls
>>         support JSON responses?  Really wouldn't be happy with that.
>>
>>
>>     ​I removed the "type" parameter in this particular call, since it
>>     made no sense - the type of the object is specified by the URL in
>>     REST fashion.
>>
>>     All the REST endpoints will support JSON responses, and it will be
>>     easily done by decorating them with the "template_renderer"
>>     decorator. This will be done in a subsequent patch.
>>
>>     I do not follow the remark about "magic" URLs. All URLs are
>>     documented​ and follow-able in REST fashion:
>>
>>     /projects/  - list all projects, or create new project (by POST)
>>     /project/ID/ - list project by id, or update project data (by POST)
>>     etc...
>>
>>     the idea is that the URLs called without the *format* parameter
>>     will return text/html, and the same URLs called with the
>>     *format=json* parameter will return application/json, in a
>>
>>     predictible fashion
>>
>>
>>         If you're wanting to avoid having a query for the projects
>>         html page and another copy of it for the API response why not
>>         use a defined Queryset? the django QuerySet managers are
>>         exactly for this.
>>
>>
>>     ​What I want to avoid is view code replication, at all levels.
>>     e.g. the computation for compatible layer versions are done
>>     separately two times, with nearly identical code - when computing
>>     suggestions for returning JSON files, and when displaying project
>>     layers table. There is no reason for this code duplication, and
>>     for different API entry points.
>>
>>     This isn't just about selecting objects from the database, but
>>     also about processing commands from the user - I think we should
>>     not have dual views for whenever we need to render HTML or JSON -
>>     we only need to be preoccupied with computing the correct data,
>>     and let the presentation layer - in this case the
>>     template_renderer decorator - worry about presenting the data in a
>>     suitable format.
>>
>>
>>
>>         +      /* 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;
>>
>>
>>         What happens if something else accesses those afterwards? If
>>         you select a project then close the popup you've potentially
>>         broken any other "user" of that context data.
>>         Ideally those properties are read-only.
>>
>>
>>     ​They were already modifiable by ​building URLs by appending
>>     specific IDs to base_ URLs - in this sense, the change only makes
>>     evident that the URLs are not static at all. Furthermore, I agree
>>     that we shouldn't build URLs by hand in JS code, so getting the
>>      URLs in complete form from the backend is cleaner, IMHO.
>>
>>     ​Since these URLs ​live in the context of the page, they always
>>     operate on the current project set in the context, no matter the
>>     caller. I think this is in accordance with the current design of
>>     the "New Build" button, which sets a current Project in page
>>     context for follow-up actions to be taken on it - it would be
>>     strange if any pieces of code would want to operate on another
>>     project without reflecting this to the user.
>>
>>
>>
>>         +class RedirectException(Exception):
>>         +    def __init__(self, view, g, mandatory_parameters, *args,
>>         **kwargs):
>>         +        super(RedirectException, self).__init__()
>>         +        self.view = view
>>         +        self.g = g
>>         +        self.mandatory_parameters = mandatory_parameters
>>         +        self.oargs  = args
>>         +        self.okwargs = kwargs
>>         +
>>         +    def get_redirect_response(self):
>>         +        return _redirect_parameters(self.view, self.g,
>>         self.mandatory_parameters, self.oargs, self.okwargs)
>>         +
>>
>>         Using HTTP redirects are a real pain because they get cached
>>         by the browser and are very difficult to invalidate.
>>
>>
>>     ​The redirects are now made temporary.
>>
>>     I think it's nicer to provide the user with defaults for options
>>     they haven't selected, and I know no better way to reflect that to
>>     the user but by issueing a redirect.​
>>
>>
>>
>>         -    def projects(request):
>>         -        template="projects.html"
>>         +    def template_renderer(template):
>>         +        def func_wrapper(view):
>>         +            def returned_wrapper(request, *args, **kwargs):
>>         +                try:
>>         +                    context = view(request, *args, **kwargs)
>>         +                except RedirectException as e:
>>         +                    return e.get_redirect_response()
>>         +
>>         +                if request.GET.get('format', None) == 'json':
>>         +                    # objects is a special keyword - it's a
>>         Page, but we need the actual objects here
>>         +                    # in XHR, the objects come in the "list"
>>         property
>>         +                    if "objects" in context:
>>         +                        context["list"] =
>>         context["objects"].object_list
>>         +                        del context["objects"]
>>         +
>>         +                    # convert separately the values that are
>>         JSON-serializable
>>         +                    json_safe = {}
>>         +                    for k in context:
>>         +                        try:
>>         + jsonfilter(context[k])
>>         +                            json_safe[k] = context[k]
>>         +                        except TypeError as te:
>>         +                            # hackity-hacky: we serialize the
>>         structure using a default serializer handler, then load
>>         +                            # it from JSON so it can be
>>         re-serialized later in the proper context
>>
>>
>>         # hackity-hacky !!
>>
>>         We're doing re-factoring to remove hacks and awkward code,
>>         let's not be adding them back in at the same rate.
>>
>>
>>     ​I've changed this to a cleaner version with no hacks.
>>     ​
>>
>>
>>         Michael
>>
>>
>>
>>
>>         On 19/05/15 16:27, Damian, Alexandru wrote:
>>
>>             Hi,
>>
>>             Can you please review this branch ? It brings in an
>>             important piece of refactoring to move Toaster towards the
>>             REST-style API that will enable a cleaner design and
>>             support for interconnection with other systems.
>>
>>             Getting into details, the "New build" button in all the
>>             pages, bar the project page, now fetches data using the
>>             projects API, which is just the "all projects" page in
>>             machine-readable format. This cleans up a bit the
>>             overloading of the xhr_datatypeahead; after the API
>>             refactoring, all xhr_ calls should be completely replaced
>>             with proper calls to the REST endpoints.
>>
>>             The code is here:
>>
>> https://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=adamian/20150515_fix_table_header
>>
>>             Incidentally, this patch fixes the "new build" breakage by
>>             the first round of URL refactoring, and the "construction"
>>             of URLs on the client side.
>>
>>             Can you please review and comment ?
>>
>>             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.
>>
>>
>>
>> ---------------------------------------------------------------------
>>         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
>>
>> ---------------------------------------------------------------------
>> 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.
>>
>>
> ---------------------------------------------------------------------
> 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

[-- Attachment #2: Type: text/html, Size: 23090 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Paul Eggleton]

Hi Alex,

On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
> On Thu, May 21, 2015 at 11:25 AM, Michael Wood <michael.g.wood@intel.com>
> wrote:
> > Conflating the web pages and the APIs into a single URL pattern/schema
> > just doesn't make sense to me because:
> > 
> > - We will have pages calling themselves with a different parameter (e.g.
> > the tables pages)
> 
> ​And this is quite ok, since it will return the same data, but in a
> different format. The whole idea of REST is to allow a unique point of
> entry for each resource - so the table data in HTML format and in JSON
> format will be at the same URI.​
> 
> > - This is not how REST frameworks are implemented in any other application
> > I've seen before
> 
> ​I've taken the browsable-api from Django REST framework as model; which
> has the same concept of exposing data in different formats based on a GET
> parameter
> 
> http://www.django-rest-framework.org/topics/browsable-api/​
> 
> > - In the future we may want to version the name-space e.g.
> > /api/1.3/projects/
> 
> And this approach will make it easier - we will only have to port a single
> set of URLs ​and not pairs for JSON and HTML content.
> 
> > - Keeping the API self contained allows for greater future flexibility
> > because it de-couples them from the page structure.
> 
> ​Exactly my point - the HTML page structure is created in templates, while
> the data itself is built in the view context; this approach enforces actual
> encapsulation of data in the context, a issue we confronted in the past.

I'm not sure you guys are talking about the same things. If this API is only 
for internal use by the application itself, fine - but if it's also for 
external applications, we probably don't want to break those external 
applications if we have to change the page URL structure. Unifying the page 
URLs and REST API URLs will force us to keep them the same, right?

> > - REST should be as self documenting as possible. Having to know which
> > URLs support JSON responses isn't helpful.
> 
> ​All REST endpoints (indeed, all URLs) will support JSON formats.​

How long do you expect this re-implementation to take? It seems like so far 
we've merged a partial refactoring which has resulted in breakage, which isn't 
really ideal; how long do you estimate we will be in this state?

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Mon, Jun 1, 2015 at 2:43 PM, Paul Eggleton <paul.eggleton@linux.intel.com
> wrote:

> Hi Alex,
>
> On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
> > On Thu, May 21, 2015 at 11:25 AM, Michael Wood <michael.g.wood@intel.com
> >
> > wrote:
> > > Conflating the web pages and the APIs into a single URL pattern/schema
> > > just doesn't make sense to me because:
> > >
> > > - We will have pages calling themselves with a different parameter
> (e.g.
> > > the tables pages)
> >
> > ​And this is quite ok, since it will return the same data, but in a
> > different format. The whole idea of REST is to allow a unique point of
> > entry for each resource - so the table data in HTML format and in JSON
> > format will be at the same URI.​
> >
> > > - This is not how REST frameworks are implemented in any other
> application
> > > I've seen before
> >
> > ​I've taken the browsable-api from Django REST framework as model; which
> > has the same concept of exposing data in different formats based on a GET
> > parameter
> >
> > http://www.django-rest-framework.org/topics/browsable-api/​
> >
> > > - In the future we may want to version the name-space e.g.
> > > /api/1.3/projects/
> >
> > And this approach will make it easier - we will only have to port a
> single
> > set of URLs ​and not pairs for JSON and HTML content.
> >
> > > - Keeping the API self contained allows for greater future flexibility
> > > because it de-couples them from the page structure.
> >
> > ​Exactly my point - the HTML page structure is created in templates,
> while
> > the data itself is built in the view context; this approach enforces
> actual
> > encapsulation of data in the context, a issue we confronted in the past.
>
> I'm not sure you guys are talking about the same things. If this API is
> only
> for internal use by the application itself, fine - but if it's also for
>

​This not just internal, but also external support.
​


> external applications, we probably don't want to break those external
> applications if we have to change the page URL structure. Unifying the page
> URLs and REST API URLs will force us to keep them the same, right?
>

​Yes, it will force us to keep them the same. It will also ease the
maintaining work.​



>
> > > - REST should be as self documenting as possible. Having to know which
> > > URLs support JSON responses isn't helpful.
> >
> > ​All REST endpoints (indeed, all URLs) will support JSON formats.​
>
> How long do you expect this re-implementation to take? It seems like so far
> we've merged a partial refactoring which has resulted in breakage, which
> isn't
> really ideal; how long do you estimate we will be in this state?
>

​I'm working on it - it should be done by Wednesday.​


>
> Cheers,
> Paul
>
> --
>
> Paul Eggleton
> Intel Open Source Technology Centre
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 4639 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Paul Eggleton]

On Monday 01 June 2015 14:49:18 Damian, Alexandru wrote:
> On Mon, Jun 1, 2015 at 2:43 PM, Paul Eggleton <paul.eggleton@linux.intel.com
> > wrote:
> > 
> > Hi Alex,
> > 
> > On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
> > > On Thu, May 21, 2015 at 11:25 AM, Michael Wood <michael.g.wood@intel.com
> > > 
> > > wrote:
> > > > Conflating the web pages and the APIs into a single URL pattern/schema
> > > > just doesn't make sense to me because:
> > > > 
> > > > - We will have pages calling themselves with a different parameter
> > 
> > (e.g.
> > 
> > > > the tables pages)
> > > 
> > > ​And this is quite ok, since it will return the same data, but in a
> > > different format. The whole idea of REST is to allow a unique point of
> > > entry for each resource - so the table data in HTML format and in JSON
> > > format will be at the same URI.​
> > > 
> > > > - This is not how REST frameworks are implemented in any other
> > 
> > application
> > 
> > > > I've seen before
> > > 
> > > ​I've taken the browsable-api from Django REST framework as model; which
> > > has the same concept of exposing data in different formats based on a
> > > GET
> > > parameter
> > > 
> > > http://www.django-rest-framework.org/topics/browsable-api/​
> > > 
> > > > - In the future we may want to version the name-space e.g.
> > > > /api/1.3/projects/
> > > 
> > > And this approach will make it easier - we will only have to port a
> > 
> > single
> > 
> > > set of URLs ​and not pairs for JSON and HTML content.
> > > 
> > > > - Keeping the API self contained allows for greater future flexibility
> > > > because it de-couples them from the page structure.
> > > 
> > > ​Exactly my point - the HTML page structure is created in templates,
> > 
> > while
> > 
> > > the data itself is built in the view context; this approach enforces
> > 
> > actual
> > 
> > > encapsulation of data in the context, a issue we confronted in the past.
> > 
> > I'm not sure you guys are talking about the same things. If this API is
> > only
> > for internal use by the application itself, fine - but if it's also for
> 
> ​This not just internal, but also external support.
> 
> > external applications, we probably don't want to break those external
> > applications if we have to change the page URL structure. Unifying the
> > page URLs and REST API URLs will force us to keep them the same, right?
> 
> ​Yes, it will force us to keep them the same. It will also ease the
> maintaining work.​

So what do we do when we need to change the URL structure for the user-facing 
side but preserve the API for existing applications?
 
Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]

On Mon, Jun 1, 2015 at 4:33 PM, Paul Eggleton <paul.eggleton@linux.intel.com
> wrote:

> On Monday 01 June 2015 14:49:18 Damian, Alexandru wrote:
> > On Mon, Jun 1, 2015 at 2:43 PM, Paul Eggleton <
> paul.eggleton@linux.intel.com
> > > wrote:
> > >
> > > Hi Alex,
> > >
> > > On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
> > > > On Thu, May 21, 2015 at 11:25 AM, Michael Wood <
> michael.g.wood@intel.com
> > > >
> > > > wrote:
> > > > > Conflating the web pages and the APIs into a single URL
> pattern/schema
> > > > > just doesn't make sense to me because:
> > > > >
> > > > > - We will have pages calling themselves with a different parameter
> > >
> > > (e.g.
> > >
> > > > > the tables pages)
> > > >
> > > > ​And this is quite ok, since it will return the same data, but in a
> > > > different format. The whole idea of REST is to allow a unique point
> of
> > > > entry for each resource - so the table data in HTML format and in
> JSON
> > > > format will be at the same URI.​
> > > >
> > > > > - This is not how REST frameworks are implemented in any other
> > >
> > > application
> > >
> > > > > I've seen before
> > > >
> > > > ​I've taken the browsable-api from Django REST framework as model;
> which
> > > > has the same concept of exposing data in different formats based on a
> > > > GET
> > > > parameter
> > > >
> > > > http://www.django-rest-framework.org/topics/browsable-api/​
> > > >
> > > > > - In the future we may want to version the name-space e.g.
> > > > > /api/1.3/projects/
> > > >
> > > > And this approach will make it easier - we will only have to port a
> > >
> > > single
> > >
> > > > set of URLs ​and not pairs for JSON and HTML content.
> > > >
> > > > > - Keeping the API self contained allows for greater future
> flexibility
> > > > > because it de-couples them from the page structure.
> > > >
> > > > ​Exactly my point - the HTML page structure is created in templates,
> > >
> > > while
> > >
> > > > the data itself is built in the view context; this approach enforces
> > >
> > > actual
> > >
> > > > encapsulation of data in the context, a issue we confronted in the
> past.
> > >
> > > I'm not sure you guys are talking about the same things. If this API is
> > > only
> > > for internal use by the application itself, fine - but if it's also for
> >
> > ​This not just internal, but also external support.
> >
> > > external applications, we probably don't want to break those external
> > > applications if we have to change the page URL structure. Unifying the
> > > page URLs and REST API URLs will force us to keep them the same, right?
> >
> > ​Yes, it will force us to keep them the same. It will also ease the
> > maintaining work.​
>
> So what do we do when we need to change the URL structure for the
> user-facing
> side but preserve the API for existing applications?
>

​My plan is to drop support for ​current API should the URL structure of
the toastergui change in radical ways. This is way less drastic than it
sounds -

The reasoning is: since this is a REST API, it closely reflects the inner
concepts and objects that Toaster manipulates. We're going to radically
change the URL structure only if the way that Toaster operates changes
dramatically. At that point, maintaining support for a backward-compatible
way is going to be a very difficult affair, anyway - we already have the
experience with the SimpleUI/separate API that we've just deleted.

Also, if we going to alter the Toaster capabilities in a significant way
that impacts the URL structure, the existing code in ToasterGUI application
is not going to cut it - at this point, is going to be far easier to add a
new application (e.g. toastergui2 ) to hold new code which can expose a
different URL structure if needed.

So, in short, if the URL structure needs changing, and we need to support
 a backward compatible API, the changes are going to happen in a new
application.



>
> Cheers,
> Paul
>
> --
>
> Paul Eggleton
> Intel Open Source Technology Centre
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 6418 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]

I have pushed an extra patch that adds JSON support to all Project-based
URLS.

To be remarked, the "importlayer" and "newproject" views are not converted
- even if the code "fits" - because they are not REST-style APIs.

The "unmanaged" versions, returning "landing_not_managed" are converted
just for illustrations, I am working on another patchset that drops those
views as part of the "unmanaged" code drop.




On Tue, Jun 2, 2015 at 10:11 AM, Damian, Alexandru <
alexandru.damian@intel.com> wrote:

>
>
> On Mon, Jun 1, 2015 at 4:33 PM, Paul Eggleton <
> paul.eggleton@linux.intel.com> wrote:
>
>> On Monday 01 June 2015 14:49:18 Damian, Alexandru wrote:
>> > On Mon, Jun 1, 2015 at 2:43 PM, Paul Eggleton <
>> paul.eggleton@linux.intel.com
>> > > wrote:
>> > >
>> > > Hi Alex,
>> > >
>> > > On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
>> > > > On Thu, May 21, 2015 at 11:25 AM, Michael Wood <
>> michael.g.wood@intel.com
>> > > >
>> > > > wrote:
>> > > > > Conflating the web pages and the APIs into a single URL
>> pattern/schema
>> > > > > just doesn't make sense to me because:
>> > > > >
>> > > > > - We will have pages calling themselves with a different parameter
>> > >
>> > > (e.g.
>> > >
>> > > > > the tables pages)
>> > > >
>> > > > ​And this is quite ok, since it will return the same data, but in a
>> > > > different format. The whole idea of REST is to allow a unique point
>> of
>> > > > entry for each resource - so the table data in HTML format and in
>> JSON
>> > > > format will be at the same URI.​
>> > > >
>> > > > > - This is not how REST frameworks are implemented in any other
>> > >
>> > > application
>> > >
>> > > > > I've seen before
>> > > >
>> > > > ​I've taken the browsable-api from Django REST framework as model;
>> which
>> > > > has the same concept of exposing data in different formats based on
>> a
>> > > > GET
>> > > > parameter
>> > > >
>> > > > http://www.django-rest-framework.org/topics/browsable-api/​
>> > > >
>> > > > > - In the future we may want to version the name-space e.g.
>> > > > > /api/1.3/projects/
>> > > >
>> > > > And this approach will make it easier - we will only have to port a
>> > >
>> > > single
>> > >
>> > > > set of URLs ​and not pairs for JSON and HTML content.
>> > > >
>> > > > > - Keeping the API self contained allows for greater future
>> flexibility
>> > > > > because it de-couples them from the page structure.
>> > > >
>> > > > ​Exactly my point - the HTML page structure is created in templates,
>> > >
>> > > while
>> > >
>> > > > the data itself is built in the view context; this approach enforces
>> > >
>> > > actual
>> > >
>> > > > encapsulation of data in the context, a issue we confronted in the
>> past.
>> > >
>> > > I'm not sure you guys are talking about the same things. If this API
>> is
>> > > only
>> > > for internal use by the application itself, fine - but if it's also
>> for
>> >
>> > ​This not just internal, but also external support.
>> >
>> > > external applications, we probably don't want to break those external
>> > > applications if we have to change the page URL structure. Unifying the
>> > > page URLs and REST API URLs will force us to keep them the same,
>> right?
>> >
>> > ​Yes, it will force us to keep them the same. It will also ease the
>> > maintaining work.​
>>
>> So what do we do when we need to change the URL structure for the
>> user-facing
>> side but preserve the API for existing applications?
>>
>
> ​My plan is to drop support for ​current API should the URL structure of
> the toastergui change in radical ways. This is way less drastic than it
> sounds -
>
> The reasoning is: since this is a REST API, it closely reflects the inner
> concepts and objects that Toaster manipulates. We're going to radically
> change the URL structure only if the way that Toaster operates changes
> dramatically. At that point, maintaining support for a backward-compatible
> way is going to be a very difficult affair, anyway - we already have the
> experience with the SimpleUI/separate API that we've just deleted.
>
> Also, if we going to alter the Toaster capabilities in a significant way
> that impacts the URL structure, the existing code in ToasterGUI application
> is not going to cut it - at this point, is going to be far easier to add a
> new application (e.g. toastergui2 ) to hold new code which can expose a
> different URL structure if needed.
>
> So, in short, if the URL structure needs changing, and we need to support
>  a backward compatible API, the changes are going to happen in a new
> application.
>
>
>
>>
>> Cheers,
>> Paul
>>
>> --
>>
>> Paul Eggleton
>> Intel Open Source Technology Centre
>>
>
>
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 8066 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Michael Wood]

If you're still really sure this is a good idea you will need to fix the 
current regressions introduced in the front end code which consumes 
these APIs.

- New build button doesn't work and New build button changes libtoaster 
ctx vars which leak a project change into other users of this value
- Layerdetails page doesn't work

Maybe a better way round this problem is to make a urls.py scanner which 
generates a js file, then we essentially have a client side reverse() 
function useable from js

e.g.

import toastergui.urls as urls

import re

args_regex = re.compile(r"(\(\?P\<(.*?)\>.*?\))")

for url in urls.urlpatterns:
     args = args_regex.findall(url.regex.pattern)
     js_url = url.regex.pattern
     for arg in args:
         js_url = js_url.replace(*arg)
     print js_url

and so on...

Michael

On 02/06/15 12:32, Damian, Alexandru wrote:
> I have pushed an extra patch that adds JSON support to all 
> Project-based URLS.
>
> To be remarked, the "importlayer" and "newproject" views are not 
> converted - even if the code "fits" - because they are not REST-style 
> APIs.
>
> The "unmanaged" versions, returning "landing_not_managed" are 
> converted just for illustrations, I am working on another patchset 
> that drops those views as part of the "unmanaged" code drop.
>
>
>
>
> On Tue, Jun 2, 2015 at 10:11 AM, Damian, Alexandru 
> <alexandru.damian@intel.com <mailto:alexandru.damian@intel.com>> wrote:
>
>
>
>     On Mon, Jun 1, 2015 at 4:33 PM, Paul Eggleton
>     <paul.eggleton@linux.intel.com
>     <mailto:paul.eggleton@linux.intel.com>> wrote:
>
>         On Monday 01 June 2015 14:49:18 Damian, Alexandru wrote:
>         > On Mon, Jun 1, 2015 at 2:43 PM, Paul Eggleton
>         <paul.eggleton@linux.intel.com
>         <mailto:paul.eggleton@linux.intel.com>
>         > > wrote:
>         > >
>         > > Hi Alex,
>         > >
>         > > On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
>         > > > On Thu, May 21, 2015 at 11:25 AM, Michael Wood
>         <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>
>         > > >
>         > > > wrote:
>         > > > > Conflating the web pages and the APIs into a single
>         URL pattern/schema
>         > > > > just doesn't make sense to me because:
>         > > > >
>         > > > > - We will have pages calling themselves with a
>         different parameter
>         > >
>         > > (e.g.
>         > >
>         > > > > the tables pages)
>         > > >
>         > > > ​And this is quite ok, since it will return the same
>         data, but in a
>         > > > different format. The whole idea of REST is to allow a
>         unique point of
>         > > > entry for each resource - so the table data in HTML
>         format and in JSON
>         > > > format will be at the same URI.​
>         > > >
>         > > > > - This is not how REST frameworks are implemented in
>         any other
>         > >
>         > > application
>         > >
>         > > > > I've seen before
>         > > >
>         > > > ​I've taken the browsable-api from Django REST framework
>         as model; which
>         > > > has the same concept of exposing data in different
>         formats based on a
>         > > > GET
>         > > > parameter
>         > > >
>         > > > http://www.django-rest-framework.org/topics/browsable-api/​
>         > > >
>         > > > > - In the future we may want to version the name-space e.g.
>         > > > > /api/1.3/projects/
>         > > >
>         > > > And this approach will make it easier - we will only
>         have to port a
>         > >
>         > > single
>         > >
>         > > > set of URLs ​and not pairs for JSON and HTML content.
>         > > >
>         > > > > - Keeping the API self contained allows for greater
>         future flexibility
>         > > > > because it de-couples them from the page structure.
>         > > >
>         > > > ​Exactly my point - the HTML page structure is created
>         in templates,
>         > >
>         > > while
>         > >
>         > > > the data itself is built in the view context; this
>         approach enforces
>         > >
>         > > actual
>         > >
>         > > > encapsulation of data in the context, a issue we
>         confronted in the past.
>         > >
>         > > I'm not sure you guys are talking about the same things.
>         If this API is
>         > > only
>         > > for internal use by the application itself, fine - but if
>         it's also for
>         >
>         > ​This not just internal, but also external support.
>         >
>         > > external applications, we probably don't want to break
>         those external
>         > > applications if we have to change the page URL structure.
>         Unifying the
>         > > page URLs and REST API URLs will force us to keep them the
>         same, right?
>         >
>         > ​Yes, it will force us to keep them the same. It will also
>         ease the
>         > maintaining work.​
>
>         So what do we do when we need to change the URL structure for
>         the user-facing
>         side but preserve the API for existing applications?
>
>
>     ​My plan is to drop support for ​current API should the URL
>     structure of the toastergui change in radical ways. This is way
>     less drastic than it sounds -
>
>     The reasoning is: since this is a REST API, it closely reflects
>     the inner concepts and objects that Toaster manipulates. We're
>     going to radically change the URL structure only if the way that
>     Toaster operates changes dramatically. At that point, maintaining
>     support for a backward-compatible way is going to be a very
>     difficult affair, anyway - we already have the experience with the
>     SimpleUI/separate API that we've just deleted.
>
>     Also, if we going to alter the Toaster capabilities in a
>     significant way that impacts the URL structure, the existing code
>     in ToasterGUI application is not going to cut it - at this point,
>     is going to be far easier to add a new application (e.g.
>     toastergui2 ) to hold new code which can expose a different URL
>     structure if needed.
>
>     So, in short, if the URL structure needs changing, and we need to
>     support  a backward compatible API, the changes are going to
>     happen in a new application.
>
>
>         Cheers,
>         Paul
>
>         --
>
>         Paul Eggleton
>         Intel Open Source Technology Centre
>
>
>
>
>     -- 
>     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.
>

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 8449 bytes --]

On Wed, Jun 3, 2015 at 12:50 PM, Michael Wood <michael.g.wood@intel.com>
wrote:

>
> If you're still really sure this is a good idea you will need to fix the
> current regressions introduced in the front end code which consumes these
> APIs.
>
> - New build button doesn't work and New build button changes libtoaster
> ctx vars which leak a project change into other users of this value
> - Layerdetails page doesn't work
>

​Yep, working on these.



>
> Maybe a better way round this problem is to make a urls.py scanner which
> generates a js file, then we essentially have a client side reverse()
> function useable from js
>
> e.g.
>
> import toastergui.urls as urls
>
> import re
>
> args_regex = re.compile(r"(\(\?P\<(.*?)\>.*?\))")
>
> for url in urls.urlpatterns:
>     args = args_regex.findall(url.regex.pattern)
>     js_url = url.regex.pattern
>     for arg in args:
>         js_url = js_url.replace(*arg)
>     print js_url
>
> and so on...
>

​Great idea, I already struggled with this to generate URLs for ​automated
HTML5 validation. I'll implement something like this and dump it in a
context in base.html.



>
> Michael
>
> On 02/06/15 12:32, Damian, Alexandru wrote:
>
>> I have pushed an extra patch that adds JSON support to all Project-based
>> URLS.
>>
>> To be remarked, the "importlayer" and "newproject" views are not
>> converted - even if the code "fits" - because they are not REST-style APIs.
>>
>> The "unmanaged" versions, returning "landing_not_managed" are converted
>> just for illustrations, I am working on another patchset that drops those
>> views as part of the "unmanaged" code drop.
>>
>>
>>
>>
>> On Tue, Jun 2, 2015 at 10:11 AM, Damian, Alexandru <
>> alexandru.damian@intel.com <mailto:alexandru.damian@intel.com>> wrote:
>>
>>
>>
>>     On Mon, Jun 1, 2015 at 4:33 PM, Paul Eggleton
>>     <paul.eggleton@linux.intel.com
>>     <mailto:paul.eggleton@linux.intel.com>> wrote:
>>
>>         On Monday 01 June 2015 14:49:18 Damian, Alexandru wrote:
>>         > On Mon, Jun 1, 2015 at 2:43 PM, Paul Eggleton
>>         <paul.eggleton@linux.intel.com
>>         <mailto:paul.eggleton@linux.intel.com>
>>         > > wrote:
>>         > >
>>         > > Hi Alex,
>>         > >
>>         > > On Friday 29 May 2015 14:58:57 Damian, Alexandru wrote:
>>         > > > On Thu, May 21, 2015 at 11:25 AM, Michael Wood
>>         <michael.g.wood@intel.com <mailto:michael.g.wood@intel.com>
>>
>>         > > >
>>         > > > wrote:
>>         > > > > Conflating the web pages and the APIs into a single
>>         URL pattern/schema
>>         > > > > just doesn't make sense to me because:
>>         > > > >
>>         > > > > - We will have pages calling themselves with a
>>         different parameter
>>         > >
>>         > > (e.g.
>>         > >
>>         > > > > the tables pages)
>>         > > >
>>         > > > ​And this is quite ok, since it will return the same
>>         data, but in a
>>         > > > different format. The whole idea of REST is to allow a
>>         unique point of
>>         > > > entry for each resource - so the table data in HTML
>>         format and in JSON
>>         > > > format will be at the same URI.​
>>         > > >
>>         > > > > - This is not how REST frameworks are implemented in
>>         any other
>>         > >
>>         > > application
>>         > >
>>         > > > > I've seen before
>>         > > >
>>         > > > ​I've taken the browsable-api from Django REST framework
>>         as model; which
>>         > > > has the same concept of exposing data in different
>>         formats based on a
>>         > > > GET
>>         > > > parameter
>>         > > >
>>         > > > http://www.django-rest-framework.org/topics/browsable-api/​
>>         > > >
>>         > > > > - In the future we may want to version the name-space e.g.
>>         > > > > /api/1.3/projects/
>>         > > >
>>         > > > And this approach will make it easier - we will only
>>         have to port a
>>         > >
>>         > > single
>>         > >
>>         > > > set of URLs ​and not pairs for JSON and HTML content.
>>         > > >
>>         > > > > - Keeping the API self contained allows for greater
>>         future flexibility
>>         > > > > because it de-couples them from the page structure.
>>         > > >
>>         > > > ​Exactly my point - the HTML page structure is created
>>         in templates,
>>         > >
>>         > > while
>>         > >
>>         > > > the data itself is built in the view context; this
>>         approach enforces
>>         > >
>>         > > actual
>>         > >
>>         > > > encapsulation of data in the context, a issue we
>>         confronted in the past.
>>         > >
>>         > > I'm not sure you guys are talking about the same things.
>>         If this API is
>>         > > only
>>         > > for internal use by the application itself, fine - but if
>>         it's also for
>>         >
>>         > ​This not just internal, but also external support.
>>         >
>>         > > external applications, we probably don't want to break
>>         those external
>>         > > applications if we have to change the page URL structure.
>>         Unifying the
>>         > > page URLs and REST API URLs will force us to keep them the
>>         same, right?
>>         >
>>         > ​Yes, it will force us to keep them the same. It will also
>>         ease the
>>         > maintaining work.​
>>
>>         So what do we do when we need to change the URL structure for
>>         the user-facing
>>         side but preserve the API for existing applications?
>>
>>
>>     ​My plan is to drop support for ​current API should the URL
>>     structure of the toastergui change in radical ways. This is way
>>     less drastic than it sounds -
>>
>>     The reasoning is: since this is a REST API, it closely reflects
>>     the inner concepts and objects that Toaster manipulates. We're
>>     going to radically change the URL structure only if the way that
>>     Toaster operates changes dramatically. At that point, maintaining
>>     support for a backward-compatible way is going to be a very
>>     difficult affair, anyway - we already have the experience with the
>>     SimpleUI/separate API that we've just deleted.
>>
>>     Also, if we going to alter the Toaster capabilities in a
>>     significant way that impacts the URL structure, the existing code
>>     in ToasterGUI application is not going to cut it - at this point,
>>     is going to be far easier to add a new application (e.g.
>>     toastergui2 ) to hold new code which can expose a different URL
>>     structure if needed.
>>
>>     So, in short, if the URL structure needs changing, and we need to
>>     support  a backward compatible API, the changes are going to
>>     happen in a new application.
>>
>>
>>         Cheers,
>>         Paul
>>
>>         --
>>
>>         Paul Eggleton
>>         Intel Open Source Technology Centre
>>
>>
>>
>>
>>     --     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.
>>
>>
> ---------------------------------------------------------------------
> 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

[-- Attachment #2: Type: text/html, Size: 11728 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

Taken for submission with the last extra patch from Michael.

Cheers,
Alex

On Fri, May 15, 2015 at 5:52 PM, Michael Wood <michael.g.wood@intel.com>
wrote:

>
> This wouldn't be valid html as it'll do:
>
> <thead id="tableheader">
>     <th>dog</th>
> </thead>
>
> When it should be
>
> <thead id="tableheader">
>    <tr>
>        <th>dog</th>
>     </tr>
> </thead>
>
> Just put an empty <th> in the <tr> that should be valid then.
>
> diff --git a/bitbake/lib/toaster/toastergui/templates/toastertable.html
> b/bitbake/lib/toaster/toastergui/templates/toastertable.html
> index 598d295..d68d093 100644
> --- a/bitbake/lib/toaster/toastergui/templates/toastertable.html
> +++ b/bitbake/lib/toaster/toastergui/templates/toastertable.html
> @@ -93,6 +93,7 @@
>    <!-- The actual table -->
>    <table class="table table-bordered table-hover tablesorter"
> id="{{table_name}}">
>      <thead>
> +      <tr><th></th></tr>
>      </thead>
>      <tbody></tbody>
>    </table>
>
>
> Michael
>
>
> On 15/05/15 14:45, Damian, Alexandru wrote:
>
>> Hi,
>>
>> This patch brings back table header in the toastertable - based pages.
>>
>> Can you please review ?
>>
>> Thank you,
>> Alex
>>
>> --
>> Alex Damian
>> Yocto Project
>> SSG / OTC
>>
>>
>>
> --
> _______________________________________________
> toaster mailing list
> toaster@yoctoproject.org
> https://lists.yoctoproject.org/listinfo/toaster
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 2804 bytes --]

Re: [review-request] adamian/20150515_fix_table_header

[Michael Wood]

This wouldn't be valid html as it'll do:

<thead id="tableheader">
     <th>dog</th>
</thead>

When it should be

<thead id="tableheader">
    <tr>
        <th>dog</th>
     </tr>
</thead>

Just put an empty <th> in the <tr> that should be valid then.

diff --git a/bitbake/lib/toaster/toastergui/templates/toastertable.html 
b/bitbake/lib/toaster/toastergui/templates/toastertable.html
index 598d295..d68d093 100644
--- a/bitbake/lib/toaster/toastergui/templates/toastertable.html
+++ b/bitbake/lib/toaster/toastergui/templates/toastertable.html
@@ -93,6 +93,7 @@
    <!-- The actual table -->
    <table class="table table-bordered table-hover tablesorter" 
id="{{table_name}}">
      <thead>
+      <tr><th></th></tr>
      </thead>
      <tbody></tbody>
    </table>


Michael

On 15/05/15 14:45, Damian, Alexandru wrote:
> Hi,
>
> This patch brings back table header in the toastertable - based pages.
>
> Can you please review ?
>
> Thank you,
> Alex
>
> -- 
> Alex Damian
> Yocto Project
> SSG / OTC
>
>

[review-request] adamian/20150515_fix_table_header

[Damian, Alexandru]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]

Hi,

This patch brings back table header in the toastertable - based pages.

Can you please review ?

Thank you,
Alex

-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 712 bytes --]