Merge lp:~sinzui/launchpad/pageids into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Curtis Hovey | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 16028 | ||||
Proposed branch: | lp:~sinzui/launchpad/pageids | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
209 lines (+81/-44) 5 files modified
lib/lp/services/inlinehelp/README.txt (+13/-7) lib/lp/services/inlinehelp/zcml.py (+2/-1) lib/lp/services/webapp/doc/webapp-publication.txt (+0/-33) lib/lp/services/webapp/publication.py (+12/-3) lib/lp/services/webapp/tests/test_pageid.py (+54/-0) |
||||
To merge this branch: | bzr merge lp:~sinzui/launchpad/pageids | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Benji York (community) | code | Approve | |
Review via email: mp+126081@code.launchpad.net |
Commit message
Make sensible pageids for help folders and ++model++.
Description of the change
The page performance report cannot handle Page-Ids with random/changing information like revisionids in URLs. Launchpad's
standard views always provide sensible __name__ and __class__.__name__ attrs
to construct them. Two cases where views are dynamically generated for
templates and resources do not provide the expected attributes, or the
code does not know how to find the data.
-------
RULES
Pre-
* HelpFolders are missing the __name__ and that is trivial to register
with the dynamic view class.
* The ++model++ example is a case were the context is also a view, and
it is dynamically generated from a page template. Either the
registration can modified, or constructPageID() must learn how to
handle the recursive issue.
* Handling at least one-level of recursion will prevent future breaks
for future dynamic view classes from happening.
QA
* Visit https:/
* Verify the Page-Id is just "RootObject:
path
* Visit https:/
* Expect a timeout, view the oops.
* Verify the Page-id is Distribution:
no path to the template.
LINT
lib/
lib/
lib/
lib/
lib/
LoC:
This add about 40 lines of code, but I have a 3000+ line credit.
TEST
./bin/test -vvc -t README lp.services.
./bin/test -vvc -t PageID lp.services.
./bin/test -vvc -t webapp-
IMPLEMENTATION
Use the name from the zcml as the help folder's __name__ attr.
lib/
lib/
Recuse through constructPageID() when the context is a dynamic view
created from a page template. I converted from doctests to unittests.
lib/
lib/
lib/
Looks good. A couple of minor comments:
As a reader of the code, I would appreciate a comment above line 122 of the
diff describing the significance of having a space in the context name.
Line 122-124 of the diff contains the comment
# This is a view of a generated view class,
# such as ++model++ view of Product:+bugs. Recuse!
Is "Recuse" a typo? It seems to me that it should be "recurse".
I suppose the function is recusing itself from working further, but that's a
stretch. :)