From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by yocto-www.yoctoproject.org (Postfix, from userid 118) id D7865E00956; Thu, 21 May 2015 03:25:06 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on yocto-www.yoctoproject.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-HAM-Report: * -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] * -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low * trust * [74.125.82.52 listed in list.dnswl.org] Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by yocto-www.yoctoproject.org (Postfix) with ESMTP id 2B5B5E004F6 for ; Thu, 21 May 2015 03:25:02 -0700 (PDT) Received: by wgfl8 with SMTP id l8so80765509wgf.2 for ; Thu, 21 May 2015 03:25:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=ewjgyccAKgf0WKkDg0eXI5w8VhGMZgHYWbd1CCL58KI=; b=kQZDJ9cRO1F6B+qAI/wTzuKikDbf8tclNdjzjluw8JNnrOYeTbI5Zajn4+/wwCv3Pa xXYbbHsXnfqDYI8q5a+znEo9SJsub5o51ZH4n2NvZsQLvOBo0C0t/G7Qkyt62oKAygIZ dd1dLVHVHIhlcfBqlmsXTtS2V8iTmCJ6PcnS5IGo6H7NClBlWVUzvXF8WZTQ9DE9I2Jd c0nl38Mx72HxVd6fJeNzrLNf+0Qk2iZ3Z+PLSHtqvgFhV+1uioiW7lREh2YjZ7XZcTul 5vMemyveVB0gzd5RWMU1xdQUp2muoW5Cm4K1nep/SM3nbRledYpT8EUZFCBCtTbY8O5+ 0ehA== X-Gm-Message-State: ALoCoQmeF6zjK2a94PAtGRDVqZLbWGRKMKCgesIjxXlgUokc/oXzi9Ixx9YfuKNICH8EhvTKGFLu X-Received: by 10.180.99.231 with SMTP id et7mr51099530wib.23.1432203901937; Thu, 21 May 2015 03:25:01 -0700 (PDT) Received: from [192.168.2.36] ([83.217.123.106]) by mx.google.com with ESMTPSA id i6sm31390457wjf.29.2015.05.21.03.25.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 May 2015 03:25:01 -0700 (PDT) Message-ID: <555DB27C.1050608@intel.com> Date: Thu, 21 May 2015 11:25:00 +0100 From: Michael Wood User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: "Damian, Alexandru" References: <555B656C.9040101@intel.com> In-Reply-To: Cc: "toaster@yoctoproject.org" Subject: Re: [review-request] adamian/20150515_fix_table_header X-BeenThere: toaster@yoctoproject.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Web based interface for BitBake List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 May 2015 10:25:06 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > 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 > > 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. >