[Toaster] [review-request] adamian/20150515_fix_table_header
Damian, Alexandru
alexandru.damian at intel.com
Thu May 21 03:43:15 PDT 2015
Reverting the last patch from submission in order to further discuss it
Alex
On Thu, May 21, 2015 at 11:25 AM, Michael Wood <michael.g.wood at intel.com>
wrote:
> Hi,
>
> Conflating the web pages and the APIs into a single URL pattern/schema
> just doesn't make sense to me because:
>
> - We will have pages calling themselves with a different parameter (e.g.
> the tables pages)
> - This is not how REST frameworks are implemented in any other application
> I've seen before
> - In the future we may want to version the name-space e.g.
> /api/1.3/projects/
> - Keeping the API self contained allows for greater future flexibility
> because it de-couples them from the page structure.
> - REST should be as self documenting as possible. Having to know which
> URLs support JSON responses isn't helpful.
> - The tree of objects in a web page doesn't not have a 1:1 mapping with
> the API so they functionally different, in only a few cases do we have 1
> page = 1 set of data.
> - The duplication is just having a url entry and a view that has a
> query+json response (you could easily do this as subclassed view,
> ToasterJsonView.as_view(QuerySet)) really that's not a lot. If you use
> pre-defined querysets that's even easier, the abstraction can make the
> duplication negligible.
> - Soon the API calls are going to be the main way in which toaster get's
> it's data and the template mechanism is going to be used very lightly.
>
> There are still a few other issues in the patch, but I think we need to
> get this sorted first as it affects everything else.
>
> I'd be much happier to have along these lines
>
> /api/projects/
> /api/project/1/
> /api/project/1/?bitbake="core-image-minimal"
> .
> .
>
> Current ToasterTable handles this already (could be tweaked to have a
> different modes):
>
> /api/project/1/compatible/machines/
> /api/project/1/compatible/layers/
> /api/project/1/compatible/recipes/
> .
> .
>
> To add (though we currently only have a use case for layer at the moment):
>
> /api/project/1/compatible/layer/3
> /api/project/1/compatible/machine/3
> /api/project/1/compatible/recipe/3
> .
> .
>
>
> Michael
>
>
>
> On 21/05/15 11:12, Damian, Alexandru wrote:
>
>> Taken for submission,
>>
>> Cheers,
>> Alex
>>
>> On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru <
>> alexandru.damian at intel.com <mailto: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 <mailto: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
>>
>> ---------------------------------------------------------------------
>> 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/20150521/4313f8a9/attachment-0001.html>
More information about the toaster
mailing list