[Toaster] [review-request] adamian/20150515_fix_table_header
Damian, Alexandru
alexandru.damian at intel.com
Thu May 21 03:12:45 PDT 2015
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
>
> "* 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
>
--
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150521/ea329ef9/attachment-0001.html>
More information about the toaster
mailing list