On 21/05/2015 11:12, "Damian, Alexandru" <[email protected]> wrote:
>Taken for submission, > > >Cheers, > >Alex > > > >On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru ><[email protected]> 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 ><[email protected]> 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 > > > -- _______________________________________________ toaster mailing list [email protected] https://lists.yoctoproject.org/listinfo/toaster
