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 > > "* 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/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
-- _______________________________________________ toaster mailing list [email protected] https://lists.yoctoproject.org/listinfo/toaster
