[Toaster] [review-request] adamian/20150515_fix_table_header
Damian, Alexandru
alexandru.damian at intel.com
Wed May 20 06:38:09 PDT 2015
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 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/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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150520/f973cad7/attachment-0001.html>
More information about the toaster
mailing list