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