[Toaster] [review-request] adamian/20150515_fix_table_header
Barros Pena, Belen
belen.barros.pena at intel.com
Thu May 21 03:23:58 PDT 2015
On 21/05/2015 11:12, "Damian, Alexandru" <alexandru.damian at intel.com>
wrote:
>Taken for submission,
>
>
>Cheers,
>
>Alex
>
>
>
>On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru
><alexandru.damian at intel.com> 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
><michael.g.wood at 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/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
>
>
>
More information about the toaster
mailing list