Re: [review-request] adamian/20150515_fix_table_header

[Michael Wood <michael.g.wood@intel.com>]

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.
>