Taken for submission,
Cheers,
Alex
On Wed, May 20, 2015 at 2:38 PM, Damian, Alexandru
<[email protected] <mailto:[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] <mailto:[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
---------------------------------------------------------------------
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.
