launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18802
Re: [Merge] lp:~blr/launchpad/ui-project-setbranch into lp:launchpad
Review: Needs Fixing code
Diff comments:
> === modified file 'LICENSE'
> --- LICENSE 2015-03-24 13:36:23 +0000
> +++ LICENSE 2015-06-15 08:42:22 +0000
> @@ -13,6 +13,8 @@
> The Launchpad name and logo are trademarks of Canonical, and may not
> be used without the prior written permission of Canonical.
>
> +Git SCM logos are licensed Creative Commons Attribution 3.0 Unported.
> +
> Third-party copyright in this distribution is noted where applicable.
>
> All rights not expressly granted are reserved.
> @@ -683,3 +685,277 @@
> <http://www.gnu.org/licenses/>.
>
> =========================================================================
> +
> +
> +Creative Commons Attribution 3.0 Unported License
> +
> +THE WORK (AS DEFINED BELOW) IS PROVIDED UNDER THE TERMS OF THIS
> +CREATIVE COMMONS PUBLIC LICENSE ("CCPL" OR "LICENSE"). THE WORK IS
> +PROTECTED BY COPYRIGHT AND/OR OTHER APPLICABLE LAW. ANY USE OF THE
> +WORK OTHER THAN AS AUTHORIZED UNDER THIS LICENSE OR COPYRIGHT LAW IS
> +PROHIBITED.
> +
> +BY EXERCISING ANY RIGHTS TO THE WORK PROVIDED HERE, YOU ACCEPT AND
> +AGREE TO BE BOUND BY THE TERMS OF THIS LICENSE. TO THE EXTENT THIS
> +LICENSE MAY BE CONSIDERED TO BE A CONTRACT, THE LICENSOR GRANTS YOU
> +THE RIGHTS CONTAINED HERE IN CONSIDERATION OF YOUR ACCEPTANCE OF SUCH
> +TERMS AND CONDITIONS.
> +
> +1. Definitions
> +
> +"Adaptation" means a work based upon the Work, or upon the Work and
> +other pre-existing works, such as a translation, adaptation,
> +derivative work, arrangement of music or other alterations of a
> +literary or artistic work, or phonogram or performance and includes
> +cinematographic adaptations or any other form in which the Work may be
> +recast, transformed, or adapted including in any form recognizably
> +derived from the original, except that a work that constitutes a
> +Collection will not be considered an Adaptation for the purpose of
> +this License. For the avoidance of doubt, where the Work is a musical
> +work, performance or phonogram, the synchronization of the Work in
> +timed-relation with a moving image ("synching") will be considered an
> +Adaptation for the purpose of this License. "Collection" means a
> +collection of literary or artistic works, such as encyclopedias and
> +anthologies, or performances, phonograms or broadcasts, or other works
> +or subject matter other than works listed in Section 1(f) below,
> +which, by reason of the selection and arrangement of their contents,
> +constitute intellectual creations, in which the Work is included in
> +its entirety in unmodified form along with one or more other
> +contributions, each constituting separate and independent works in
> +themselves, which together are assembled into a collective whole. A
> +work that constitutes a Collection will not be considered an
> +Adaptation (as defined above) for the purposes of this License.
> +"Distribute" means to make available to the public the original and
> +copies of the Work or Adaptation, as appropriate, through sale or
> +other transfer of ownership. "Licensor" means the individual,
> +individuals, entity or entities that offer(s) the Work under the terms
> +of this License. "Original Author" means, in the case of a literary or
> +artistic work, the individual, individuals, entity or entities who
> +created the Work or if no individual or entity can be identified, the
> +publisher; and in addition (i) in the case of a performance the
> +actors, singers, musicians, dancers, and other persons who act, sing,
> +deliver, declaim, play in, interpret or otherwise perform literary or
> +artistic works or expressions of folklore; (ii) in the case of a
> +phonogram the producer being the person or legal entity who first
> +fixes the sounds of a performance or other sounds; and, (iii) in the
> +case of broadcasts, the organization that transmits the broadcast.
> +"Work" means the literary and/or artistic work offered under the terms
> +of this License including without limitation any production in the
> +literary, scientific and artistic domain, whatever may be the mode or
> +form of its expression including digital form, such as a book,
> +pamphlet and other writing; a lecture, address, sermon or other work
> +of the same nature; a dramatic or dramatico-musical work; a
> +choreographic work or entertainment in dumb show; a musical
> +composition with or without words; a cinematographic work to which are
> +assimilated works expressed by a process analogous to cinematography;
> +a work of drawing, painting, architecture, sculpture, engraving or
> +lithography; a photographic work to which are assimilated works
> +expressed by a process analogous to photography; a work of applied
> +art; an illustration, map, plan, sketch or three-dimensional work
> +relative to geography, topography, architecture or science; a
> +performance; a broadcast; a phonogram; a compilation of data to the
> +extent it is protected as a copyrightable work; or a work performed by
> +a variety or circus performer to the extent it is not otherwise
> +considered a literary or artistic work. "You" means an individual or
> +entity exercising rights under this License who has not previously
> +violated the terms of this License with respect to the Work, or who
> +has received express permission from the Licensor to exercise rights
> +under this License despite a previous violation. "Publicly Perform"
> +means to perform public recitations of the Work and to communicate to
> +the public those public recitations, by any means or process,
> +including by wire or wireless means or public digital performances; to
> +make available to the public Works in such a way that members of the
> +public may access these Works from a place and at a place individually
> +chosen by them; to perform the Work to the public by any means or
> +process and the communication to the public of the performances of the
> +Work, including by public digital performance; to broadcast and
> +rebroadcast the Work by any means including signs, sounds or images.
> +"Reproduce" means to make copies of the Work by any means including
> +without limitation by sound or visual recordings and the right of
> +fixation and reproducing fixations of the Work, including storage of a
> +protected performance or phonogram in digital form or other electronic
> +medium. 2. Fair Dealing Rights. Nothing in this License is intended to
> +reduce, limit, or restrict any uses free from copyright or rights
> +arising from limitations or exceptions that are provided for in
> +connection with the copyright protection under copyright law or other
> +applicable laws.
> +
> +3. License Grant. Subject to the terms and conditions of this License,
> +Licensor hereby grants You a worldwide, royalty-free, non-exclusive,
> +perpetual (for the duration of the applicable copyright) license to
> +exercise the rights in the Work as stated below:
> +
> +to Reproduce the Work, to incorporate the Work into one or more
> +Collections, and to Reproduce the Work as incorporated in the
> +Collections; to create and Reproduce Adaptations provided that any
> +such Adaptation, including any translation in any medium, takes
> +reasonable steps to clearly label, demarcate or otherwise identify
> +that changes were made to the original Work. For example, a
> +translation could be marked "The original work was translated from
> +English to Spanish," or a modification could indicate "The original
> +work has been modified."; to Distribute and Publicly Perform the Work
> +including as incorporated in Collections; and, to Distribute and
> +Publicly Perform Adaptations. For the avoidance of doubt:
> +
> +Non-waivable Compulsory License Schemes. In those jurisdictions in
> +which the right to collect royalties through any statutory or
> +compulsory licensing scheme cannot be waived, the Licensor reserves
> +the exclusive right to collect such royalties for any exercise by You
> +of the rights granted under this License; Waivable Compulsory License
> +Schemes. In those jurisdictions in which the right to collect
> +royalties through any statutory or compulsory licensing scheme can be
> +waived, the Licensor waives the exclusive right to collect such
> +royalties for any exercise by You of the rights granted under this
> +License; and, Voluntary License Schemes. The Licensor waives the right
> +to collect royalties, whether individually or, in the event that the
> +Licensor is a member of a collecting society that administers
> +voluntary licensing schemes, via that society, from any exercise by
> +You of the rights granted under this License. The above rights may be
> +exercised in all media and formats whether now known or hereafter
> +devised. The above rights include the right to make such modifications
> +as are technically necessary to exercise the rights in other media and
> +formats. Subject to Section 8(f), all rights not expressly granted by
> +Licensor are hereby reserved.
> +
> +4. Restrictions. The license granted in Section 3 above is expressly
> +made subject to and limited by the following restrictions:
> +
> +You may Distribute or Publicly Perform the Work only under the terms
> +of this License. You must include a copy of, or the Uniform Resource
> +Identifier (URI) for, this License with every copy of the Work You
> +Distribute or Publicly Perform. You may not offer or impose any terms
> +on the Work that restrict the terms of this License or the ability of
> +the recipient of the Work to exercise the rights granted to that
> +recipient under the terms of the License. You may not sublicense the
> +Work. You must keep intact all notices that refer to this License and
> +to the disclaimer of warranties with every copy of the Work You
> +Distribute or Publicly Perform. When You Distribute or Publicly
> +Perform the Work, You may not impose any effective technological
> +measures on the Work that restrict the ability of a recipient of the
> +Work from You to exercise the rights granted to that recipient under
> +the terms of the License. This Section 4(a) applies to the Work as
> +incorporated in a Collection, but this does not require the Collection
> +apart from the Work itself to be made subject to the terms of this
> +License. If You create a Collection, upon notice from any Licensor You
> +must, to the extent practicable, remove from the Collection any credit
> +as required by Section 4(b), as requested. If You create an
> +Adaptation, upon notice from any Licensor You must, to the extent
> +practicable, remove from the Adaptation any credit as required by
> +Section 4(b), as requested. If You Distribute, or Publicly Perform the
> +Work or any Adaptations or Collections, You must, unless a request has
> +been made pursuant to Section 4(a), keep intact all copyright notices
> +for the Work and provide, reasonable to the medium or means You are
> +utilizing: (i) the name of the Original Author (or pseudonym, if
> +applicable) if supplied, and/or if the Original Author and/or Licensor
> +designate another party or parties (e.g., a sponsor institute,
> +publishing entity, journal) for attribution ("Attribution Parties") in
> +Licensor's copyright notice, terms of service or by other reasonable
> +means, the name of such party or parties; (ii) the title of the Work
> +if supplied; (iii) to the extent reasonably practicable, the URI, if
> +any, that Licensor specifies to be associated with the Work, unless
> +such URI does not refer to the copyright notice or licensing
> +information for the Work; and (iv) , consistent with Section 3(b), in
> +the case of an Adaptation, a credit identifying the use of the Work in
> +the Adaptation (e.g., "French translation of the Work by Original
> +Author," or "Screenplay based on original Work by Original Author").
> +The credit required by this Section 4 (b) may be implemented in any
> +reasonable manner; provided, however, that in the case of a Adaptation
> +or Collection, at a minimum such credit will appear, if a credit for
> +all contributing authors of the Adaptation or Collection appears, then
> +as part of these credits and in a manner at least as prominent as the
> +credits for the other contributing authors. For the avoidance of
> +doubt, You may only use the credit required by this Section for the
> +purpose of attribution in the manner set out above and, by exercising
> +Your rights under this License, You may not implicitly or explicitly
> +assert or imply any connection with, sponsorship or endorsement by the
> +Original Author, Licensor and/or Attribution Parties, as appropriate,
> +of You or Your use of the Work, without the separate, express prior
> +written permission of the Original Author, Licensor and/or Attribution
> +Parties. Except as otherwise agreed in writing by the Licensor or as
> +may be otherwise permitted by applicable law, if You Reproduce,
> +Distribute or Publicly Perform the Work either by itself or as part of
> +any Adaptations or Collections, You must not distort, mutilate, modify
> +or take other derogatory action in relation to the Work which would be
> +prejudicial to the Original Author's honor or reputation. Licensor
> +agrees that in those jurisdictions (e.g. Japan), in which any exercise
> +of the right granted in Section 3(b) of this License (the right to
> +make Adaptations) would be deemed to be a distortion, mutilation,
> +modification or other derogatory action prejudicial to the Original
> +Author's honor and reputation, the Licensor will waive or not assert,
> +as appropriate, this Section, to the fullest extent permitted by the
> +applicable national law, to enable You to reasonably exercise Your
> +right under Section 3(b) of this License (right to make Adaptations)
> +but not otherwise. 5. Representations, Warranties and Disclaimer
> +
> +UNLESS OTHERWISE MUTUALLY AGREED TO BY THE PARTIES IN WRITING,
> +LICENSOR OFFERS THE WORK AS-IS AND MAKES NO REPRESENTATIONS OR
> +WARRANTIES OF ANY KIND CONCERNING THE WORK, EXPRESS, IMPLIED,
> +STATUTORY OR OTHERWISE, INCLUDING, WITHOUT LIMITATION, WARRANTIES OF
> +TITLE, MERCHANTIBILITY, FITNESS FOR A PARTICULAR PURPOSE,
> +NONINFRINGEMENT, OR THE ABSENCE OF LATENT OR OTHER DEFECTS, ACCURACY,
> +OR THE PRESENCE OF ABSENCE OF ERRORS, WHETHER OR NOT DISCOVERABLE.
> +SOME JURISDICTIONS DO NOT ALLOW THE EXCLUSION OF IMPLIED WARRANTIES,
> +SO SUCH EXCLUSION MAY NOT APPLY TO YOU.
> +
> +6. Limitation on Liability. EXCEPT TO THE EXTENT REQUIRED BY
> +APPLICABLE LAW, IN NO EVENT WILL LICENSOR BE LIABLE TO YOU ON ANY
> +LEGAL THEORY FOR ANY SPECIAL, INCIDENTAL, CONSEQUENTIAL, PUNITIVE OR
> +EXEMPLARY DAMAGES ARISING OUT OF THIS LICENSE OR THE USE OF THE WORK,
> +EVEN IF LICENSOR HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGES.
> +
> +7. Termination
> +
> +This License and the rights granted hereunder will terminate
> +automatically upon any breach by You of the terms of this License.
> +Individuals or entities who have received Adaptations or Collections
> +from You under this License, however, will not have their licenses
> +terminated provided such individuals or entities remain in full
> +compliance with those licenses. Sections 1, 2, 5, 6, 7, and 8 will
> +survive any termination of this License. Subject to the above terms
> +and conditions, the license granted here is perpetual (for the
> +duration of the applicable copyright in the Work). Notwithstanding the
> +above, Licensor reserves the right to release the Work under different
> +license terms or to stop distributing the Work at any time; provided,
> +however that any such election will not serve to withdraw this License
> +(or any other license that has been, or is required to be, granted
> +under the terms of this License), and this License will continue in
> +full force and effect unless terminated as stated above. 8.
> +Miscellaneous
> +
> +Each time You Distribute or Publicly Perform the Work or a Collection,
> +the Licensor offers to the recipient a license to the Work on the same
> +terms and conditions as the license granted to You under this License.
> +Each time You Distribute or Publicly Perform an Adaptation, Licensor
> +offers to the recipient a license to the original Work on the same
> +terms and conditions as the license granted to You under this License.
> +If any provision of this License is invalid or unenforceable under
> +applicable law, it shall not affect the validity or enforceability of
> +the remainder of the terms of this License, and without further action
> +by the parties to this agreement, such provision shall be reformed to
> +the minimum extent necessary to make such provision valid and
> +enforceable. No term or provision of this License shall be deemed
> +waived and no breach consented to unless such waiver or consent shall
> +be in writing and signed by the party to be charged with such waiver
> +or consent. This License constitutes the entire agreement between the
> +parties with respect to the Work licensed here. There are no
> +understandings, agreements or representations with respect to the Work
> +not specified here. Licensor shall not be bound by any additional
> +provisions that may appear in any communication from You. This License
> +may not be modified without the mutual written agreement of the
> +Licensor and You. The rights granted under, and the subject matter
> +referenced, in this License were drafted utilizing the terminology of
> +the Berne Convention for the Protection of Literary and Artistic Works
> +(as amended on September 28, 1979), the Rome Convention of 1961, the
> +WIPO Copyright Treaty of 1996, the WIPO Performances and Phonograms
> +Treaty of 1996 and the Universal Copyright Convention (as revised on
> +July 24, 1971). These rights and subject matter take effect in the
> +relevant jurisdiction in which the License terms are sought to be
> +enforced according to the corresponding provisions of the
> +implementation of those treaty provisions in the applicable national
> +law. If the standard suite of rights granted under applicable
> +copyright law includes additional rights not granted under this
> +License, such additional rights are deemed to be included in the
> +License; this License is not intended to restrict the license of any
> +rights under applicable law.
> +
> +=========================================================================
> \ No newline at end of file
>
> === modified file 'lib/canonical/launchpad/icing/inline-sprites-1.css.in'
> --- lib/canonical/launchpad/icing/inline-sprites-1.css.in 2015-03-24 13:36:23 +0000
> +++ lib/canonical/launchpad/icing/inline-sprites-1.css.in 2015-06-15 08:42:22 +0000
> @@ -84,7 +84,11 @@
> .branch {
> background-image: url(/@@/branch.png); /* sprite-ref: icon-sprites */
> background-repeat: no-repeat;
> - }
> +}
> +.gitbranch {
> + background-image: url(/@@/gitbranch.png); /* sprite-ref: icon-sprites */
> + background-repeat: no-repeat;
> +}
> .distribution {
> background-image: url(/@@/distribution.png); /* sprite-ref: icon-sprites */
> background-repeat: no-repeat;
>
> === modified file 'lib/canonical/launchpad/icing/style.css'
> --- lib/canonical/launchpad/icing/style.css 2015-04-07 23:43:43 +0000
> +++ lib/canonical/launchpad/icing/style.css 2015-06-15 08:42:22 +0000
> @@ -64,6 +64,10 @@
> padding-left: 1em;
> }
>
> +form label {
> + font-weight: bold;
> +}
> +
The widgets seem to all be in a table, and there's already a "form table label" rule, so is this necessary?
> form.primary.search {
> margin-bottom: 2em;
> }
> @@ -537,7 +541,31 @@
>
>
> /* --- Code --- */
> -
> +code.command {
> + background-color: #fff;
> + border: 1px solid #ddd;
> + border-radius: 3px;
> + box-sizing: border-box;
Does box-sizing do anything useful here? We're not explicitly sizing it AFAICS.
> + color: #626262;
> + padding: 4px;
> + font-family: "DejaVu Sans Mono", "Courier New", monospace;
> + font-size: 1.05em;
> +}
> +code.command-block {
> + display: block;
> + margin-bottom: 1em;
> + padding: 6px;
> +}
> +code.command-block:last-child {
> + margin: 0;
> +}
> +div.scm-tip {
> + overflow: hidden;
When's this useful?
> + padding: 5px;
This indents the push instructions 5px more than the others.
> +}
> +div#product-push-instructions {
> + width: 50em;
> +}
This isn't used any more.
> table.code {
> margin-bottom: 1em;
> }
>
> === added file 'lib/canonical/launchpad/images/gitbranch-large.png'
> Binary files lib/canonical/launchpad/images/gitbranch-large.png 1970-01-01 00:00:00 +0000 and lib/canonical/launchpad/images/gitbranch-large.png 2015-06-15 08:42:22 +0000 differ
> === added file 'lib/canonical/launchpad/images/gitbranch.png'
> Binary files lib/canonical/launchpad/images/gitbranch.png 1970-01-01 00:00:00 +0000 and lib/canonical/launchpad/images/gitbranch.png 2015-06-15 08:42:22 +0000 differ
> === modified file 'lib/lp/code/browser/branchlisting.py'
> --- lib/lp/code/browser/branchlisting.py 2015-06-15 05:28:12 +0000
> +++ lib/lp/code/browser/branchlisting.py 2015-06-15 08:42:22 +0000
> @@ -1134,9 +1134,9 @@
> def configure_codehosting(self):
> """Get the menu link for configuring code hosting."""
> if not check_permission(
> - 'launchpad.Edit', self.context.development_focus):
> + 'launchpad.Edit', self.context):
This'll fit on one line.
> return None
> - series_menu = MenuAPI(self.context.development_focus).overview
> + series_menu = MenuAPI(self.context).overview
> set_branch = series_menu['set_branch']
> set_branch.text = 'Configure Code'
> return set_branch
This isn't the series_menu any more. And can the property be shrunk/removed now that it's basically just accessing a menu item on the context object? eg. project-translations.pt uses context/menu:translations/settings/fmt:link.
>
> === modified file 'lib/lp/code/javascript/productseries-setbranch.js'
> --- lib/lp/code/javascript/productseries-setbranch.js 2015-05-19 03:01:06 +0000
> +++ lib/lp/code/javascript/productseries-setbranch.js 2015-06-15 08:42:22 +0000
> @@ -24,6 +24,19 @@
> return selected;
> };
>
> + module._get_selected_default_vcs = function () {
> + var vcs = document.getElementsByName('field.default_vcs');
> + var selectedDefaultVCS;
> + var i;
> + for (i = 0; i < vcs.length; i++) {
> + if (vcs[i].checked) {
> + selectedDefaultVCS = vcs[i].value;
> + break;
This might as well just return directly. No need for a local variable or a break.
> + }
> + }
> + return selectedDefaultVCS;
> + };
> +
>
> module.__rcs_types = null;
>
> @@ -39,6 +52,36 @@
> field.disabled = !is_enabled;
> };
>
> + module.setup_expanders = function() {
> + var git_content_node = Y.one('#git-expander-content');
> + var git_expander = new Y.lp.app.widgets.expander.Expander(
> + Y.one('#git-expander-icon'), git_content_node,
> + {animate_node: git_content_node}
> + );
> +
> + var bzr_content_node = Y.one('#bzr-expander-content');
> + var bzr_expander = new Y.lp.app.widgets.expander.Expander(
> + Y.one('#bzr-expander-icon'), bzr_content_node,
> + {animate_node: bzr_content_node }
> + );
> + module.git_expander = git_expander.setUp();
> + module.bzr_expander = bzr_expander.setUp();
animate_node defaults to content_node. You can drop the third parameter and inline *_content_node in both cases.
> + };
> +
> + module.onclick_default_vcs = function(e) {
> + /* Which project vcs was selected? */
> + var selectedDefaultVCS =
> + module._get_selected_default_vcs();
> +
> + if (selectedDefaultVCS === 'GIT') {
> + module.git_expander.render(true);
> + module.bzr_expander.render(false);
> + } else {
> + module.bzr_expander.render(true);
> + module.git_expander.render(false);
> + }
> + };
> +
> module.onclick_rcs_type = function(e) {
> /* Which rcs type radio button has been selected? */
> // CVS
> @@ -81,10 +124,14 @@
> 'click', module.onclick_rcs_type);
> Y.all('input[name="field.branch_type"]').on(
> 'click', module.onclick_branch_type);
> + Y.all('input[name="field.default_vcs"]').on(
> + 'click', module.onclick_default_vcs);
>
> // Set the initial state.
> + module.setup_expanders();
> module.onclick_rcs_type();
> module.onclick_branch_type();
> + module.onclick_default_vcs();
> };
>
> }, "0.1", {"requires": ["node", "DOM"]});
>
> === modified file 'lib/lp/code/stories/codeimport/xx-create-codeimport.txt'
> --- lib/lp/code/stories/codeimport/xx-create-codeimport.txt 2015-06-15 05:28:12 +0000
> +++ lib/lp/code/stories/codeimport/xx-create-codeimport.txt 2015-06-15 08:42:22 +0000
> @@ -38,11 +38,12 @@
> >>> owner_browser = setupBrowser(auth="Basic test@xxxxxxxxxxxxx:test")
> >>> owner_browser.open('http://code.launchpad.dev/firefox')
> >>> owner_browser.getLink('Configure Code').click()
> + >>> owner_browser.getControl('Bazaar', index=0).click()
> >>> owner_browser.getControl(
> ... 'Import a branch').click()
> >>> owner_browser.getControl('Branch URL').value = (
> ... 'git://example.com/firefox')
> - >>> owner_browser.getControl('Git').click()
> + >>> owner_browser.getControl('Git', index=1).click()
> >>> owner_browser.getControl('Branch name').value = 'trunk'
> >>> owner_browser.getControl('Update').click()
>
>
> === modified file 'lib/lp/code/templates/product-branch-summary.pt'
> --- lib/lp/code/templates/product-branch-summary.pt 2015-06-04 06:54:09 +0000
> +++ lib/lp/code/templates/product-branch-summary.pt 2015-06-15 08:42:22 +0000
> @@ -56,8 +56,7 @@
> </p>
> </div>
>
> - <tal:no-branches
> - condition="not: view/branch_count">
> + <tal:no-branches condition="not: view/branch_count">
> There are no branches for <tal:project-name replace="context/displayname"/>
> in Launchpad.
> <tal:can-configure condition="view/can_configure_branches">
>
> === modified file 'lib/lp/registry/browser/configure.zcml'
> --- lib/lp/registry/browser/configure.zcml 2015-06-10 10:19:25 +0000
> +++ lib/lp/registry/browser/configure.zcml 2015-06-15 08:42:22 +0000
> @@ -1752,6 +1752,20 @@
> template="../templates/product-review-license.pt"
> />
> <browser:page
> + name="+setbranch"
Let's go for consistency and call it +configure-code, since it also configures the project VCS and Git repositories, not just a branch.
> + for="lp.registry.interfaces.product.IProduct"
> + class="lp.registry.browser.product.ProductSetBranchView"
> + permission="launchpad.Edit"
> + template="../templates/project-setbranch.pt"
> + />
This is a Code-specific view, so it should be defined and registered under lib/lp/code.
Also ensure it's inside a facet="branches" block. It currently appears as part of the "Overview" tab.
> + <browser:page
> + name="+product-macros"
> + for="lp.registry.interfaces.product.IProduct"
> + class="lp.app.browser.launchpad.Macro"
> + permission="zope.Public"
> + template="../templates/product-macros.pt"
> + />
> + <browser:page
> name="+edit"
> for="lp.registry.interfaces.nameblacklist.INameBlacklist"
> class="lp.registry.browser.nameblacklist.NameBlacklistEditView"
>
> === modified file 'lib/lp/registry/browser/product.py'
> --- lib/lp/registry/browser/product.py 2015-06-15 05:28:12 +0000
> +++ lib/lp/registry/browser/product.py 2015-06-15 08:42:22 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Browser views for products."""
> @@ -30,6 +30,7 @@
> 'ProductRdfView',
> 'ProductReviewLicenseView',
> 'ProductSeriesSetView',
> + 'ProductSetBranchView',
> 'ProductSetBreadcrumb',
> 'ProductSetNavigation',
> 'ProductSetReviewLicensesView',
> @@ -44,8 +45,12 @@
>
> from operator import attrgetter
>
> +from bzrlib.revision import NULL_REVISION
> from lazr.delegates import delegates
> -from lazr.restful.interface import copy_field
> +from lazr.restful.interface import (
> + copy_field,
> + use_template,
> + )
> from lazr.restful.interfaces import IJSONRequestCache
> from z3c.ptcompat import ViewPageTemplateFile
> from zope.component import getUtility
> @@ -66,6 +71,7 @@
> from zope.schema import (
> Bool,
> Choice,
> + TextLine,
> )
> from zope.schema.vocabulary import (
> SimpleTerm,
> @@ -80,6 +86,7 @@
> custom_widget,
> LaunchpadEditFormView,
> LaunchpadFormView,
> + render_radio_widget_part,
> ReturnToReferrerMixin,
> safe_action,
> )
> @@ -103,7 +110,10 @@
> PUBLIC_PROPRIETARY_INFORMATION_TYPES,
> ServiceUsage,
> )
> -from lp.app.errors import NotFoundError
> +from lp.app.errors import (
> + NotFoundError,
> + UnexpectedFormData,
> + )
> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
> from lp.app.utilities import json_dump_information_types
> from lp.app.vocabularies import InformationTypeVocabulary
> @@ -131,9 +141,31 @@
> StructuralSubscriptionTargetTraversalMixin,
> )
> from lp.bugs.interfaces.bugtask import RESOLVED_BUGTASK_STATUSES
> +from lp.code.browser.branch import BranchNameValidationMixin
> from lp.code.browser.branchref import BranchRef
> +from lp.code.browser.codeimport import validate_import_url
> from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
> +from lp.code.enums import (
> + BranchType,
> + RevisionControlSystems,
> + )
> +from lp.code.errors import (
> + BranchCreationForbidden,
> + BranchExists,
> + GitTargetError,
> + )
> +from lp.code.interfaces.branch import IBranch
> +from lp.code.interfaces.branchcollection import IBranchCollection
> +from lp.code.interfaces.branchjob import IRosettaUploadJobSource
> +from lp.code.interfaces.branchtarget import IBranchTarget
> +from lp.code.interfaces.codeimport import (
> + ICodeImport,
> + ICodeImportSet,
> + )
> +from lp.code.interfaces.gitcollection import IGitCollection
> +from lp.code.interfaces.gitrepository import IGitRepositorySet
> from lp.code.browser.vcslisting import TargetDefaultVCSNavigationMixin
> +
> from lp.registry.browser import (
> add_subscribe_link,
> BaseRdfView,
> @@ -150,6 +182,7 @@
> PillarNavigationMixin,
> PillarViewMixin,
> )
> +from lp.registry.enums import VCSType
> from lp.registry.interfaces.pillar import IPillarNameSet
> from lp.registry.interfaces.product import (
> IProduct,
> @@ -171,6 +204,7 @@
> from lp.services.fields import (
> PillarAliases,
> PublicPersonChoice,
> + URIField,
> )
> from lp.services.librarian.interfaces import ILibraryFileAliasSet
> from lp.services.propertycache import cachedproperty
> @@ -347,7 +381,6 @@
> 'configured' -- a boolean representing the configuration status.
> """
> overview_menu = MenuAPI(self.context).overview
> - series_menu = MenuAPI(self.context.development_focus).overview
> configuration_names = [
> 'configure_bugtracker',
> 'configure_translations',
> @@ -363,7 +396,7 @@
> configured=config_statuses[key]))
>
> # Add the branch configuration in separately.
> - set_branch = series_menu['set_branch']
> + set_branch = overview_menu['set_branch']
> set_branch.text = 'Code'
> set_branch.summary = "Specify the location of this project's code."
> config_list.insert(0,
> @@ -506,6 +539,7 @@
> 'packages',
> 'series',
> 'series_add',
> + 'set_branch',
> 'milestones',
> 'downloads',
> 'announce',
> @@ -559,6 +593,19 @@
> 'RDF</abbr> metadata')
> return Link('+rdf', text, icon='download')
>
> + @enabled_with_permission('launchpad.Edit')
> + def set_branch(self):
> + """Return a link to set the branch or repo for this project."""
> + if self.context.development_focus.branch is None:
> + text = 'Link to branch'
> + icon = 'add'
> + summary = 'Set the branch for this project'
> + else:
> + text = "Change branch"
> + icon = 'edit'
> + summary = 'Change the branch for this project'
> + return Link('+setbranch', text, summary, icon=icon)
Checking development_focus.branch isn't sufficient any more, as the Git repository may be set instead.
But we should take this opportunity to fix the (+) vs (!) silliness. All the others are always edit links that just say "Configure $APP".
> +
> def downloads(self):
> text = 'Downloads'
> return Link('+download', text, icon='info')
> @@ -938,6 +985,30 @@
> return self.context.license_status != LicenseStatus.OPEN_SOURCE
>
> @property
> + def show_vcs(self):
> + """Should VCS be displayed?
> +
> + Infer a project VCS for this view if a git or bzr branch
> + exist, otherwise check if vcs attribute has been set.
> + """
> + if (not IBranchCollection(self.context).is_empty() or
> + not IGitCollection(self.context).is_empty()):
> + return True
> + return bool(self.context.vcs)
This could be dropped, and the template updated to just use view/vcs.
> +
> + @property
> + def vcs(self):
> + """Project VCS type."""
> + vcs = None
> + if self.context.vcs:
> + return self.context.vcs
> + if not IBranchCollection(self.context).is_empty():
> + vcs = VCSType.BZR
> + if not IGitCollection(self.context).is_empty():
> + vcs = VCSType.GIT
> + return vcs
This is the sort of thing that might be useful elsewhere, so it probably makes sense to push it down to the model.
> +
> + @property
> def sourceforge_url(self):
> if self.context.sourceforgeproject:
> return ("http://sourceforge.net/projects/%s";
> @@ -1594,6 +1665,372 @@
> return BatchNavigator(decorated_result, self.request)
>
>
> +LINK_LP_BZR = 'link-lp-bzr'
> +IMPORT_EXTERNAL = 'import-external'
> +
> +
> +BRANCH_TYPE_VOCABULARY = SimpleVocabulary((
> + SimpleTerm(LINK_LP_BZR, LINK_LP_BZR,
> + _("Link to a Bazaar branch already on Launchpad")),
> + SimpleTerm(IMPORT_EXTERNAL, IMPORT_EXTERNAL,
> + _("Import a branch hosted somewhere else")),
> + ))
> +
> +class SetBranchForm(Interface):
> + """The fields presented on the form for setting a branch."""
> +
> + use_template(ICodeImport, ['cvs_module'])
> +
> + default_vcs = Choice(title=_("Project VCS"),
> + required=True, vocabulary=VCSType,
> + description=_("The version control system for this project."))
> +
> + rcs_type = Choice(title=_("Type of RCS"),
> + required=False, vocabulary=RevisionControlSystems,
> + description=_(
> + "The version control system to import from. "))
> +
> + repo_url = URIField(
> + title=_("Branch URL"), required=True,
> + description=_("The URL of the branch."),
> + allowed_schemes=["http", "https"],
> + allow_userinfo=False, allow_port=True, allow_query=False,
> + allow_fragment=False, trailing_slash=False)
> +
> + branch_location = copy_field(
> + IProductSeries['branch'], __name__='branch_location',
> + title=_('Branch'),
> + description=_(
> + "The Bazaar branch for this series in Launchpad, "
> + "if one exists."))
> +
> + git_repository_location = TextLine(
> + title=_(
> + 'Git Repository'),
> + required=False,
> + description=_(
> + "The Git repository for this project in Launchpad, "
> + "if one exists, in the form: "
> + "`~user/project-name/+git/repo-name`"))
Field descriptions don't support Markdown, so the backticks look a rather odd.
> +
> + branch_type = Choice(
> + title=_('Import type'), vocabulary=BRANCH_TYPE_VOCABULARY,
> + description=_("The type of import"), required=True)
> +
> + branch_name = copy_field(
> + IBranch['name'], __name__='branch_name', title=_('Branch name'),
> + description=_(''), required=True)
> +
> + branch_owner = copy_field(
> + IBranch['owner'], __name__='branch_owner', title=_('Branch owner'),
> + description=_(''), required=True)
> +
> +
> +class ProductSetBranchView(ReturnToReferrerMixin, LaunchpadFormView,
> + ProductView,
> + BranchNameValidationMixin):
> + """The view to set a branch default for the Product."""
This view should have a label of "Configure code" or similar. Currently there's no informative heading or document title.
> +
> + schema = SetBranchForm
> + # Set for_input to True to ensure fields marked read-only will be editable
> + # upon creation.
> + for_input = True
> +
> + custom_widget('rcs_type', LaunchpadRadioWidget)
> + custom_widget('branch_type', LaunchpadRadioWidget)
> + custom_widget('default_vcs', LaunchpadRadioWidget)
> +
> + errors_in_action = False
> +
> + @property
> + def series(self):
> + return self.context.development_focus
> +
> + @property
> + def vcs(self):
> + return self.context.vcs
> +
> + @property
> + def initial_values(self):
> + return dict(
> + rcs_type=RevisionControlSystems.BZR,
> + default_vcs=(self.vcs or VCSType.GIT),
> + branch_type=LINK_LP_BZR,
> + branch_location=self.series.branch)
default_vcs should start out as the project VCS, the inferred VCS, or Bazaar, in that order. We can change the default to Git when we believe our Git support is ready.
> +
> + @property
> + def next_url(self):
> + """Return the next_url.
> +
> + Use the value from `ReturnToReferrerMixin` or None if there
> + are errors.
> + """
> + if self.errors_in_action:
> + return None
> + return super(ProductSetBranchView, self).next_url
> +
> + def setUpWidgets(self):
> + """See `LaunchpadFormView`."""
> + super(ProductSetBranchView, self).setUpWidgets()
> + widget = self.widgets['rcs_type']
> + vocab = widget.vocabulary
> + current_value = widget._getFormValue()
> + self.rcs_type_cvs = render_radio_widget_part(
> + widget, vocab.CVS, current_value, 'CVS')
> + self.rcs_type_svn = render_radio_widget_part(
> + widget, vocab.BZR_SVN, current_value, 'SVN')
> + self.rcs_type_git = render_radio_widget_part(
> + widget, vocab.GIT, current_value)
> + self.rcs_type_bzr = render_radio_widget_part(
> + widget, vocab.BZR, current_value)
> + self.rcs_type_emptymarker = widget._emptyMarker()
> +
> + widget = self.widgets['branch_type']
> + current_value = widget._getFormValue()
> + vocab = widget.vocabulary
> +
> + (self.branch_type_link,
> + self.branch_type_import) = [
> + render_radio_widget_part(widget, value, current_value)
> + for value in (LINK_LP_BZR, IMPORT_EXTERNAL)]
> +
> + widget = self.widgets['default_vcs']
> + vocab = widget.vocabulary
> + current_value = widget._getFormValue()
> + self.default_vcs_git = render_radio_widget_part(
> + widget, vocab.GIT, current_value, 'Git')
> + self.default_vcs_bzr = render_radio_widget_part(
> + widget, vocab.BZR, current_value, 'Bazaar')
> +
> + def _validateLinkLpBzr(self, data):
> + """Validate data for link-lp-bzr case."""
> + if 'branch_location' not in data:
> + self.setFieldError(
> + 'branch_location', 'The branch location must be set.')
> +
> + def _validateLinkLpGit(self, data):
> + """Validate data for link-lp-git case."""
> + if data.get('git_repository_location'):
> + repo = getUtility(IGitRepositorySet).getByPath(
> + self.user, data.get('git_repository_location'))
> + if not repo:
> + self.setFieldError(
> + 'git_repository_location',
> + 'The respository does not exist.')
Validation is performed in the vocab for branch_location. This works for now, but it might not be difficult to create a trivial vocab which doesn't yet support searching.
There's also a typo in the error message.
> +
> + def _validateImportExternal(self, data):
> + """Validate data for import external case."""
> + rcs_type = data.get('rcs_type')
> + repo_url = data.get('repo_url')
> +
> + # Private teams are forbidden from owning code imports.
> + branch_owner = data.get('branch_owner')
> + if branch_owner is not None and branch_owner.private:
> + self.setFieldError(
> + 'branch_owner', 'Private teams are forbidden from owning '
> + 'external imports.')
> +
> + if repo_url is None:
> + self.setFieldError(
> + 'repo_url', 'You must set the external repository URL.')
> + else:
> + reason = validate_import_url(repo_url)
> + if reason:
> + self.setFieldError('repo_url', reason)
> +
> + # RCS type is mandatory.
> + # This condition should never happen since an initial value is set.
> + if rcs_type is None:
> + # The error shows but does not identify the widget.
> + self.setFieldError(
> + 'rcs_type',
> + 'You must specify the type of RCS for the remote host.')
> + elif rcs_type == RevisionControlSystems.CVS:
> + if 'cvs_module' not in data:
> + self.setFieldError('cvs_module', 'The CVS module must be set.')
> + self._validateBranch(data)
> +
> + def _validateBranch(self, data):
> + """Validate that branch name and owner are set."""
> + if 'branch_name' not in data:
> + self.setFieldError('branch_name', 'The branch name must be set.')
> + if 'branch_owner' not in data:
> + self.setFieldError('branch_owner', 'The branch owner must be set.')
> +
> + def _setRequired(self, names, value):
> + """Mark the widget field as optional."""
> + for name in names:
> + widget = self.widgets[name]
> + # The 'required' property on the widget context is set to False.
> + # The widget also has a 'required' property but it isn't used
> + # during validation.
> + widget.context.required = value
> +
> + def _validSchemes(self, rcs_type):
> + """Return the valid schemes for the repository URL."""
> + schemes = set(['http', 'https'])
> + # Extend the allowed schemes for the repository URL based on
> + # rcs_type.
> + extra_schemes = {
> + RevisionControlSystems.BZR_SVN: ['svn'],
> + RevisionControlSystems.GIT: ['git'],
> + RevisionControlSystems.BZR: ['bzr'],
> + }
> + schemes.update(extra_schemes.get(rcs_type, []))
> + return schemes
> +
> + def validate_widgets(self, data, names=None):
> + """See `LaunchpadFormView`."""
> + names = ['branch_type', 'rcs_type', 'default_vcs']
> + super(ProductSetBranchView, self).validate_widgets(data, names)
> + branch_type = data.get('branch_type')
> +
> + if data.get('default_vcs') == VCSType.GIT:
> + self._setRequired(['rcs_type', 'repo_url', 'cvs_module',
> + 'branch_name', 'branch_type', 'branch_owner'],
> + False)
This doesn't really make sense. The default VCS doesn't affect whether the Bazaar widgets are required or not, and it's likely to result in crashes when expected data isn't there.
> + elif branch_type == LINK_LP_BZR:
> + # Mark other widgets as non-required.
> + self._setRequired(['rcs_type', 'repo_url', 'cvs_module',
> + 'branch_name', 'branch_owner'], False)
> + elif branch_type == IMPORT_EXTERNAL:
> + rcs_type = data.get('rcs_type')
> +
> + # Set the valid schemes based on rcs_type.
> + self.widgets['repo_url'].field.allowed_schemes = (
> + self._validSchemes(rcs_type))
> + # The branch location is not required for validation.
> + self._setRequired(['branch_location'], False)
> + # The cvs_module is required if it is a CVS import.
> + if rcs_type == RevisionControlSystems.CVS:
> + self._setRequired(['cvs_module'], True)
> + else:
> + raise AssertionError("Unknown branch type %s" % branch_type)
> + # Perform full validation now.
> + super(ProductSetBranchView, self).validate_widgets(data)
> +
> + def validate(self, data):
> + """See `LaunchpadFormView`."""
> + # If widget validation returned errors then there is no need to
> + # continue as we'd likely just override the errors reported there.
> + if len(self.errors) > 0:
> + return
> + branch_type = data.get('branch_type')
> + if data.get('default_vcs') == VCSType.GIT:
> + self._validateLinkLpGit(data)
Again, default_vcs and branch_type are orthogonal. _validateLinkLpGit should always be called, as should the bzr validator appropriate to the selected branch_type.
> + elif branch_type == IMPORT_EXTERNAL:
> + self._validateImportExternal(data)
> + elif branch_type == LINK_LP_BZR:
> + self._validateLinkLpBzr(data)
> + else:
> + raise AssertionError("Unknown branch type %s" % branch_type)
> +
> + @property
> + def target(self):
> + """The branch target for the context."""
> + return IBranchTarget(self.context)
> +
> + def abort_update():
> + """Abort transaction.
> +
> + This is normally handled by LaunchpadFormView, but this can be called
> + from the success handler."""
> +
> + self.errors_in_action = True
> + self._abort()
> + return
> +
> + @action(_('Update'), name='update')
> + def update_action(self, action, data):
> + branch_type = data.get('branch_type')
> + default_vcs = data.get('default_vcs')
> +
> + self.context.vcs = default_vcs
> +
> + if default_vcs == VCSType.GIT:
This needs to be executed regardless of default_vcs.
> + git_repository_location = data.get('git_repository_location')
> + if git_repository_location:
> + repo = getUtility(IGitRepositorySet).getByPath(self.user,
> + git_repository_location)
> + try:
> + getUtility(IGitRepositorySet).setDefaultRepository(
> + self.context, repo)
> + except GitTargetError:
> + self.setFieldError(
> + 'git_repository_location',
> + 'Repository is in a different project.')
> + abort_update()
This can be checked at validation time (self.context == repo.target), so we don't need to use the abort_update hack to let us validate in update.
> +
> + elif branch_type == LINK_LP_BZR:
> + branch_location = data.get('branch_location')
> + if branch_location != self.series.branch:
> + self.series.branch = branch_location
> + # Request an initial upload of translation files.
> + getUtility(IRosettaUploadJobSource).create(
> + self.series.branch, NULL_REVISION)
> + else:
> + self.series.branch = branch_location
> + self.request.response.addInfoNotification(
> + 'Series code location updated.')
> + else:
> + branch_name = data.get('branch_name')
> + branch_owner = data.get('branch_owner')
> +
> + if branch_type == IMPORT_EXTERNAL:
> + rcs_type = data.get('rcs_type')
> + if rcs_type == RevisionControlSystems.CVS:
> + cvs_root = data.get('repo_url')
> + cvs_module = data.get('cvs_module')
> + url = None
> + else:
> + cvs_root = None
> + cvs_module = None
> + url = data.get('repo_url')
> + rcs_item = RevisionControlSystems.items[rcs_type.name]
> + try:
> + code_import = getUtility(ICodeImportSet).new(
> + owner=branch_owner,
> + registrant=self.user,
> + target=IBranchTarget(self.target),
> + branch_name=branch_name,
> + rcs_type=rcs_item,
> + url=url,
> + cvs_root=cvs_root,
> + cvs_module=cvs_module)
> + except BranchExists as e:
> + self._setBranchExists(e.existing_branch, 'branch_name')
> + abort_update()
> + self.series.branch = code_import.branch
> + self.request.response.addInfoNotification(
> + 'Code import created and branch linked to the series.')
> + else:
> + raise UnexpectedFormData(branch_type)
> +
> + def _createBzrBranch(self, branch_name, branch_owner, repo_url=None):
> + """Create a new hosted Bazaar branch.
> +
> + Return the branch on success or None.
> + """
> + branch = None
> + try:
> + namespace = self.target.getNamespace(branch_owner)
> + branch = namespace.createBranch(
> + branch_type=BranchType.HOSTED, name=branch_name,
> + registrant=self.user, url=repo_url)
> + except BranchCreationForbidden:
> + self.addError(
> + "You are not allowed to create branches in %s." %
> + self.context.displayname)
> + except BranchExists as e:
> + self._setBranchExists(e.existing_branch, 'branch_name')
> + if branch is None:
> + self.errors_in_action = True
> + # Abort transaction. This is normally handled by
> + # LaunchpadFormView, but we are already in the success handler.
> + self._abort()
> + return branch
> +
> +
> class ProductRdfView(BaseRdfView):
> """A view that sets its mime-type to application/rdf+xml"""
>
>
> === modified file 'lib/lp/registry/browser/productseries.py'
> --- lib/lp/registry/browser/productseries.py 2015-06-12 14:20:12 +0000
> +++ lib/lp/registry/browser/productseries.py 2015-06-15 08:42:22 +0000
> @@ -28,11 +28,6 @@
>
> from operator import attrgetter
>
> -from bzrlib.revision import NULL_REVISION
> -from lazr.restful.interface import (
> - copy_field,
> - use_template,
> - )
> from z3c.ptcompat import ViewPageTemplateFile
> from zope.component import getUtility
> from zope.formlib import form
> @@ -57,17 +52,11 @@
> custom_widget,
> LaunchpadEditFormView,
> LaunchpadFormView,
> - render_radio_widget_part,
> - ReturnToReferrerMixin,
> )
> from lp.app.browser.tales import MenuAPI
> from lp.app.enums import ServiceUsage
> -from lp.app.errors import (
> - NotFoundError,
> - UnexpectedFormData,
> - )
> +from lp.app.errors import NotFoundError
> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
> -from lp.app.widgets.itemswidgets import LaunchpadRadioWidget
> from lp.app.widgets.textwidgets import StrippedTextWidget
> from lp.blueprints.browser.specificationtarget import (
> HasSpecificationsMenuMixin,
> @@ -81,24 +70,9 @@
> StructuralSubscriptionTargetTraversalMixin,
> )
> from lp.bugs.interfaces.bugtask import IBugTaskSet
> -from lp.code.browser.branch import BranchNameValidationMixin
> from lp.code.browser.branchref import BranchRef
> -from lp.code.browser.codeimport import validate_import_url
> -from lp.code.enums import (
> - BranchType,
> - RevisionControlSystems,
> - )
> -from lp.code.errors import (
> - BranchCreationForbidden,
> - BranchExists,
> - )
> -from lp.code.interfaces.branch import IBranch
> -from lp.code.interfaces.branchjob import IRosettaUploadJobSource
> +from lp.code.enums import RevisionControlSystems
> from lp.code.interfaces.branchtarget import IBranchTarget
> -from lp.code.interfaces.codeimport import (
> - ICodeImport,
> - ICodeImportSet,
> - )
> from lp.registry.browser import (
> add_subscribe_link,
> BaseRdfView,
> @@ -110,6 +84,7 @@
> InvolvedMenu,
> PillarInvolvementView,
> )
> +from lp.registry.browser.product import ProductSetBranchView
> from lp.registry.errors import CannotPackageProprietaryProduct
> from lp.registry.interfaces.packaging import (
> IPackaging,
> @@ -117,7 +92,6 @@
> )
> from lp.registry.interfaces.productseries import IProductSeries
> from lp.registry.interfaces.series import SeriesStatus
> -from lp.services.fields import URIField
> from lp.services.propertycache import cachedproperty
> from lp.services.webapp import (
> ApplicationMenu,
> @@ -756,301 +730,25 @@
> self.next_url = canonical_url(product)
>
>
> -LINK_LP_BZR = 'link-lp-bzr'
> -IMPORT_EXTERNAL = 'import-external'
> -
> -
> -BRANCH_TYPE_VOCABULARY = SimpleVocabulary((
> - SimpleTerm(LINK_LP_BZR, LINK_LP_BZR,
> - _("Link to a Bazaar branch already on Launchpad")),
> - SimpleTerm(IMPORT_EXTERNAL, IMPORT_EXTERNAL,
> - _("Import a branch hosted somewhere else")),
> - ))
> -
> -
> -class SetBranchForm(Interface):
> - """The fields presented on the form for setting a branch."""
> -
> - use_template(ICodeImport, ['cvs_module'])
> -
> - rcs_type = Choice(title=_("Type of RCS"),
> - required=False, vocabulary=RevisionControlSystems,
> - description=_(
> - "The version control system to import from. "))
> -
> - repo_url = URIField(
> - title=_("Branch URL"), required=True,
> - description=_("The URL of the branch."),
> - allowed_schemes=["http", "https"],
> - allow_userinfo=False, allow_port=True, allow_query=False,
> - allow_fragment=False, trailing_slash=False)
> -
> - branch_location = copy_field(
> - IProductSeries['branch'], __name__='branch_location',
> - title=_('Branch'),
> - description=_(
> - "The Bazaar branch for this series in Launchpad, "
> - "if one exists."))
> -
> - branch_type = Choice(
> - title=_('Import type'), vocabulary=BRANCH_TYPE_VOCABULARY,
> - description=_("The type of import"), required=True)
> -
> - branch_name = copy_field(
> - IBranch['name'], __name__='branch_name', title=_('Branch name'),
> - description=_(''), required=True)
> -
> - branch_owner = copy_field(
> - IBranch['owner'], __name__='branch_owner', title=_('Branch owner'),
> - description=_(''), required=True)
> -
> -
> -class ProductSeriesSetBranchView(ReturnToReferrerMixin, LaunchpadFormView,
> - ProductSeriesView,
> - BranchNameValidationMixin):
> +class ProductSeriesSetBranchView(ProductSetBranchView, ProductSeriesView):
> """The view to set a branch for the ProductSeries."""
>
> - schema = SetBranchForm
> - # Set for_input to True to ensure fields marked read-only will be editable
> - # upon creation.
> - for_input = True
> -
> - custom_widget('rcs_type', LaunchpadRadioWidget)
> - custom_widget('branch_type', LaunchpadRadioWidget)
> -
> - errors_in_action = False
> -
> @property
> def initial_values(self):
> return dict(
> rcs_type=RevisionControlSystems.BZR,
> - branch_type=LINK_LP_BZR,
> - branch_location=self.context.branch)
> + branch_type='link-lp-bzr',
Why has this changed?
> + branch_location=self.series.branch)
self.context == self.series in this view. self.series.branch makes sense in the parent class, though.
Do we need to override initial_values here at all? The only difference from the parent class is the omission of default_vcs, but it's fine to have values for fields that don't exist.
>
> @property
> - def next_url(self):
> - """Return the next_url.
> -
> - Use the value from `ReturnToReferrerMixin` or None if there
> - are errors.
> - """
> - if self.errors_in_action:
> - return None
> - return super(ProductSeriesSetBranchView, self).next_url
> -
> - def setUpWidgets(self):
> - """See `LaunchpadFormView`."""
> - super(ProductSeriesSetBranchView, self).setUpWidgets()
> - widget = self.widgets['rcs_type']
> - vocab = widget.vocabulary
> - current_value = widget._getFormValue()
> - self.rcs_type_cvs = render_radio_widget_part(
> - widget, vocab.CVS, current_value, 'CVS')
> - self.rcs_type_svn = render_radio_widget_part(
> - widget, vocab.BZR_SVN, current_value, 'SVN')
> - self.rcs_type_git = render_radio_widget_part(
> - widget, vocab.GIT, current_value)
> - self.rcs_type_bzr = render_radio_widget_part(
> - widget, vocab.BZR, current_value)
> - self.rcs_type_emptymarker = widget._emptyMarker()
> -
> - widget = self.widgets['branch_type']
> - current_value = widget._getFormValue()
> - vocab = widget.vocabulary
> -
> - (self.branch_type_link,
> - self.branch_type_import) = [
> - render_radio_widget_part(widget, value, current_value)
> - for value in (LINK_LP_BZR, IMPORT_EXTERNAL)]
> -
> - def _validateLinkLpBzr(self, data):
> - """Validate data for link-lp-bzr case."""
> - if 'branch_location' not in data:
> - self.setFieldError(
> - 'branch_location', 'The branch location must be set.')
> -
> - def _validateImportExternal(self, data):
> - """Validate data for import external case."""
> - rcs_type = data.get('rcs_type')
> - repo_url = data.get('repo_url')
> -
> - # Private teams are forbidden from owning code imports.
> - branch_owner = data.get('branch_owner')
> - if branch_owner is not None and branch_owner.private:
> - self.setFieldError(
> - 'branch_owner', 'Private teams are forbidden from owning '
> - 'external imports.')
> -
> - if repo_url is None:
> - self.setFieldError(
> - 'repo_url', 'You must set the external repository URL.')
> - else:
> - reason = validate_import_url(repo_url, rcs_type)
> - if reason:
> - self.setFieldError('repo_url', reason)
> -
> - # RCS type is mandatory.
> - # This condition should never happen since an initial value is set.
> - if rcs_type is None:
> - # The error shows but does not identify the widget.
> - self.setFieldError(
> - 'rcs_type',
> - 'You must specify the type of RCS for the remote host.')
> - elif rcs_type == RevisionControlSystems.CVS:
> - if 'cvs_module' not in data:
> - self.setFieldError('cvs_module', 'The CVS module must be set.')
> - self._validateBranch(data)
> -
> - def _validateBranch(self, data):
> - """Validate that branch name and owner are set."""
> - if 'branch_name' not in data:
> - self.setFieldError('branch_name', 'The branch name must be set.')
> - if 'branch_owner' not in data:
> - self.setFieldError('branch_owner', 'The branch owner must be set.')
> -
> - def _setRequired(self, names, value):
> - """Mark the widget field as optional."""
> - for name in names:
> - widget = self.widgets[name]
> - # The 'required' property on the widget context is set to False.
> - # The widget also has a 'required' property but it isn't used
> - # during validation.
> - widget.context.required = value
> -
> - def _validSchemes(self, rcs_type):
> - """Return the valid schemes for the repository URL."""
> - schemes = set(['http', 'https'])
> - # Extend the allowed schemes for the repository URL based on
> - # rcs_type.
> - extra_schemes = {
> - RevisionControlSystems.BZR_SVN: ['svn'],
> - RevisionControlSystems.GIT: ['git'],
> - RevisionControlSystems.BZR: ['bzr'],
> - }
> - schemes.update(extra_schemes.get(rcs_type, []))
> - return schemes
> -
> - def validate_widgets(self, data, names=None):
> - """See `LaunchpadFormView`."""
> - names = ['branch_type', 'rcs_type']
> - super(ProductSeriesSetBranchView, self).validate_widgets(data, names)
> - branch_type = data.get('branch_type')
> - if branch_type == LINK_LP_BZR:
> - # Mark other widgets as non-required.
> - self._setRequired(['rcs_type', 'repo_url', 'cvs_module',
> - 'branch_name', 'branch_owner'], False)
> - elif branch_type == IMPORT_EXTERNAL:
> - rcs_type = data.get('rcs_type')
> -
> - # Set the valid schemes based on rcs_type.
> - self.widgets['repo_url'].field.allowed_schemes = (
> - self._validSchemes(rcs_type))
> - # The branch location is not required for validation.
> - self._setRequired(['branch_location'], False)
> - # The cvs_module is required if it is a CVS import.
> - if rcs_type == RevisionControlSystems.CVS:
> - self._setRequired(['cvs_module'], True)
> - else:
> - raise AssertionError("Unknown branch type %s" % branch_type)
> - # Perform full validation now.
> - super(ProductSeriesSetBranchView, self).validate_widgets(data)
> -
> - def validate(self, data):
> - """See `LaunchpadFormView`."""
> - # If widget validation returned errors then there is no need to
> - # continue as we'd likely just override the errors reported there.
> - if len(self.errors) > 0:
> - return
> - branch_type = data['branch_type']
> - if branch_type == IMPORT_EXTERNAL:
> - self._validateImportExternal(data)
> - elif branch_type == LINK_LP_BZR:
> - self._validateLinkLpBzr(data)
> - else:
> - raise AssertionError("Unknown branch type %s" % branch_type)
> + def series(self):
> + return self.context
>
> @property
> def target(self):
> """The branch target for the context."""
> return IBranchTarget(self.context.product)
>
> - @action(_('Update'), name='update')
> - def update_action(self, action, data):
> - branch_type = data.get('branch_type')
> - if branch_type == LINK_LP_BZR:
> - branch_location = data.get('branch_location')
> - if branch_location != self.context.branch:
> - self.context.branch = branch_location
> - # Request an initial upload of translation files.
> - getUtility(IRosettaUploadJobSource).create(
> - self.context.branch, NULL_REVISION)
> - else:
> - self.context.branch = branch_location
> - self.request.response.addInfoNotification(
> - 'Series code location updated.')
> - else:
> - branch_name = data.get('branch_name')
> - branch_owner = data.get('branch_owner')
> -
> - if branch_type == IMPORT_EXTERNAL:
> - rcs_type = data.get('rcs_type')
> - if rcs_type == RevisionControlSystems.CVS:
> - cvs_root = data.get('repo_url')
> - cvs_module = data.get('cvs_module')
> - url = None
> - else:
> - cvs_root = None
> - cvs_module = None
> - url = data.get('repo_url')
> - rcs_item = RevisionControlSystems.items[rcs_type.name]
> - try:
> - code_import = getUtility(ICodeImportSet).new(
> - owner=branch_owner,
> - registrant=self.user,
> - target=IBranchTarget(self.context.product),
> - branch_name=branch_name,
> - rcs_type=rcs_item,
> - url=url,
> - cvs_root=cvs_root,
> - cvs_module=cvs_module)
> - except BranchExists as e:
> - self._setBranchExists(e.existing_branch, 'branch_name')
> - self.errors_in_action = True
> - # Abort transaction. This is normally handled
> - # by LaunchpadFormView, but we are already in
> - # the success handler.
> - self._abort()
> - return
> - self.context.branch = code_import.branch
> - self.request.response.addInfoNotification(
> - 'Code import created and branch linked to the series.')
> - else:
> - raise UnexpectedFormData(branch_type)
> -
> - def _createBzrBranch(self, branch_name, branch_owner, repo_url=None):
> - """Create a new hosted Bazaar branch.
> -
> - Return the branch on success or None.
> - """
> - branch = None
> - try:
> - namespace = self.target.getNamespace(branch_owner)
> - branch = namespace.createBranch(
> - branch_type=BranchType.HOSTED, name=branch_name,
> - registrant=self.user, url=repo_url)
> - except BranchCreationForbidden:
> - self.addError(
> - "You are not allowed to create branches in %s." %
> - self.context.displayname)
> - except BranchExists as e:
> - self._setBranchExists(e.existing_branch, 'branch_name')
> - if branch is None:
> - self.errors_in_action = True
> - # Abort transaction. This is normally handled by
> - # LaunchpadFormView, but we are already in the success handler.
> - self._abort()
> - return branch
> -
>
> class ProductSeriesReviewView(LaunchpadEditFormView):
> """A view to review and change the series `IProduct` and name."""
>
> === modified file 'lib/lp/registry/browser/tests/test_product.py'
> --- lib/lp/registry/browser/tests/test_product.py 2014-06-19 06:38:53 +0000
> +++ lib/lp/registry/browser/tests/test_product.py 2015-06-15 08:42:22 +0000
> @@ -34,6 +34,7 @@
> from lp.registry.enums import (
> EXCLUSIVE_TEAM_POLICY,
> TeamMembershipPolicy,
> + VCSType,
> )
> from lp.registry.interfaces.product import (
> IProductSet,
> @@ -309,6 +310,12 @@
> view = create_initialized_view(self.product, '+index')
> self.assertTrue(view.show_programming_languages)
>
> + def test_show_default_vcs(self):
> + with person_logged_in(self.product.owner):
> + self.product.vcs = VCSType.GIT
> + view = create_initialized_view(self.product, '+index')
> + self.assertTrue(view.show_vcs)
> +
> def test_show_license_info_without_other_license(self):
> # show_license_info is false when one of the "other" licences is
> # not selected.
>
> === modified file 'lib/lp/registry/stories/product/xx-product-development-focus.txt'
> --- lib/lp/registry/stories/product/xx-product-development-focus.txt 2015-06-14 23:48:24 +0000
> +++ lib/lp/registry/stories/product/xx-product-development-focus.txt 2015-06-15 08:42:22 +0000
> @@ -74,7 +74,7 @@
> (http://launchpad.dev/fooix/+edit)
> >>> print_involvement_portlet(owner_browser)
> Code
> - http://launchpad.dev/fooix/trunk/+setbranch
> + http://launchpad.dev/fooix/+setbranch
> Bugs
> http://launchpad.dev/fooix/+configure-bugtracker
> Translations
> @@ -85,6 +85,7 @@
> The owner can specify the development focus branch from the overview page.
>
> >>> owner_browser.getLink(url='+setbranch').click()
> + >>> owner_browser.getControl('Bazaar', index=0).click()
> >>> owner_browser.getControl(name='field.branch_location').value = (
> ... '~eric/fooix/trunk')
> >>> owner_browser.getControl('Update').click()
>
> === modified file 'lib/lp/registry/templates/product-index.pt'
> --- lib/lp/registry/templates/product-index.pt 2015-01-29 16:28:30 +0000
> +++ lib/lp/registry/templates/product-index.pt 2015-06-15 08:42:22 +0000
> @@ -129,6 +129,11 @@
> <dd tal:content="structure view/languages_edit_widget" />
> </dl>
>
> + <dl id="product-vcs" tal:condition="view/show_vcs">
> + <dt>Version Control System:</dt>
> + <dd tal:content="view/vcs/title">Git</dd>
> + </dl>
Labels on Product:+index don't use title case (except for the buggy Programming Languages).
> +
> <dl id="licences">
> <dt>Licences:</dt>
> <dd>
>
> === added file 'lib/lp/registry/templates/product-macros.pt'
> --- lib/lp/registry/templates/product-macros.pt 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/templates/product-macros.pt 2015-06-15 08:42:22 +0000
> @@ -0,0 +1,41 @@
> +<tal:root
> + xmlns:tal="http://xml.zope.org/namespaces/tal";
> + xmlns:metal="http://xml.zope.org/namespaces/metal";
> + xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> + omit-tag="">
> +
> + <div metal:define-macro="push-instructions-git" id="push-instructions-git" class="scm-tip">
> + <h3><i class="sprite gitbranch"></i>Git Push Instructions</h3>
Why i?
Also, title case is probably excessive here.
> + <p>You can add a remote for your git branch with the
> + command:</p>
> + <p>
> + <code class="command command-block">
> + git remote add origin git+ssh://<tal:user replace="view/user/name"/>@git.launchpad.net/<tal:project replace="context/name"/><br />
This should use git_ssh_root rather than hardcoding the domain.
We don't include the username in URLs elsewhere currently, so let's not bother with it here either.
> + </code></p>
> + <p>… and push the git branch to Launchpad with:</p>
Git
> + <p>
> + <code class="command command-block">
> + git push origin master
> + </code>
> + </p>
> + </div>
> +
> + <div metal:define-macro="push-instructions-bzr" id="push-instructions-bzr" class="scm-tip">
> + <h3><i class="sprite branch"></i>Bazaar Push Instructions</h3>
> + <p>You can push a Bazaar branch directly to Launchpad with the command:</p>
> + <p>
> + <code class="command command-block">
> + bzr push lp:<tal:project replace="context/name"/>
> + </code>
> + </p>
> + </div>
Bazaar belongs above Git for now.
> +
> +
> + <tal:no-keys condition="not:view/user/sshkeys">
> + <p>To authenticate with the Launchpad branch upload service,
> + you need to
> + <a tal:attributes="href string:${view/user/fmt:url}/+editsshkeys">
> + register a SSH key</a>.</p>
> + </tal:no-keys>
This is in a macro template but not in a macro. Is it doing anything?
> +
> +</tal:root>
>
> === modified file 'lib/lp/registry/templates/productseries-setbranch.pt'
> --- lib/lp/registry/templates/productseries-setbranch.pt 2015-05-01 13:18:54 +0000
> +++ lib/lp/registry/templates/productseries-setbranch.pt 2015-06-15 08:42:22 +0000
> @@ -56,6 +56,14 @@
> Import a branch hosted somewhere else
> </label>
> <table class="subordinate">
> +
> + <tal:widget define="widget nocall:view/widgets/branch_name">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> + <tal:widget define="widget nocall:view/widgets/branch_owner">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> +
> <tal:widget define="widget nocall:view/widgets/repo_url">
> <metal:block use-macro="context/@@launchpad_form/widget_row" />
> </tal:widget>
> @@ -101,13 +109,6 @@
> </td>
> </tr>
>
> - <tal:widget define="widget nocall:view/widgets/branch_name">
> - <metal:block use-macro="context/@@launchpad_form/widget_row" />
> - </tal:widget>
> - <tal:widget define="widget nocall:view/widgets/branch_owner">
> - <metal:block use-macro="context/@@launchpad_form/widget_row" />
> - </tal:widget>
> -
> </table>
> <input tal:replace="structure view/rcs_type_emptymarker" />
Most of this template could be in a macro shared with project-setbranch.pt. Or they could even be the same template, with the Git bits just tal:conditioned out on ProductSeries:+setbranch.
>
>
> === added file 'lib/lp/registry/templates/project-setbranch.pt'
> --- lib/lp/registry/templates/project-setbranch.pt 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/templates/project-setbranch.pt 2015-06-15 08:42:22 +0000
> @@ -0,0 +1,160 @@
> +<html
> + xmlns="http://www.w3.org/1999/xhtml";
> + xmlns:tal="http://xml.zope.org/namespaces/tal";
> + xmlns:metal="http://xml.zope.org/namespaces/metal";
> + xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> + metal:use-macro="view/macro:page/main_only"
> + i18n:domain="launchpad">
> +
> + <body>
> +
> + <metal:block fill-slot="head_epilogue">
> + <style type="text/css">
> + .subordinate {
> + margin: 0.5em 0 0.5em 4em;
> + }
> + </style>
> + </metal:block>
> +
> + <div metal:fill-slot="main">
> +
> + <div metal:use-macro="context/@@launchpad_form/form">
> +
> + <metal:formbody fill-slot="widgets">
> + <h3>Version Control System</h3>
Sentence case pls.
> + <div id="default_vcs">
> + <p>Please select a vcs for your project:</p>
This label is fairly redundant with the h3.
> + <ul>
> + <li>
> + <label tal:replace="structure view/default_vcs_git">
> + Git
> + </label>
> + </li>
> + <li>
> + <label tal:replace="structure view/default_vcs_bzr">
> + Bazaar
> + </label>
> + </li>
Bazaar should be first, and it wouldn't hurt to stick a "(beta)" after the Git label.
> + </ul>
> + <p>
> + <span class="formHelp">Your project may still have both
> + git repositories and bazaar branches.</span><br/>
Git, Bazaar.
> + </p>
> + </div>
> +
> + <div id="show-hide-git">
> + <a href="#" id="git-expander-icon" class="expander-icon js-action">
> + Configure Git
> + </a>
It looks a bit weird to have:
V Configure Git
(λ) Git Push Instructions
[...]
Link Git Repository
Maybe try something like:
V (λ) Git settings
Push a new repository
[...]
Link an existing repository
A beta admonition would probably also be an idea.
> + <div id="git-expander-content">
> + <div id="form_git" class="form">
> + <div class="push-instructions">
> + <div metal:use-macro="context/@@+product-macros/push-instructions-git"></div>
> + </div>
> +
> + <h3>Link Git Repository</h3>
> + <p>Link to a Git repository already in Launchpad</p>
> + <tal:widget define="widget nocall:view/widgets/git_repository_location">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> + </div>
> + </div>
> + </div>
> +
> + <div id="show-hide-bzr">
> + <a href="#" id="bzr-expander-icon" class="expander-icon js-action">
> + Configure Bazaar
> + </a>
> + <div id="bzr-expander-content">
> + <div class="push-instructions">
> + <div metal:use-macro="context/@@+product-macros/push-instructions-bzr"></div>
> + </div>
> +
> + <table id="form_bzr" class="form">
> + <tr>
> + <td>
> + <h3>Bazaar Branch Link/Import</h3>
> + <label tal:replace="structure view/branch_type_link">
> + Link to a Bazaar branch already in Launchpad
> + </label>
> + <table class="subordinate">
> + <tal:widget define="widget nocall:view/widgets/branch_location">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> + </table>
> + </td>
> + </tr>
> +
> + <tr id="branch_mirror">
> + <td>
> + <label tal:replace="structure view/branch_type_import">
> + Import a branch hosted somewhere else
> + </label>
> + <table class="subordinate">
> + <tal:widget define="widget nocall:view/widgets/branch_name">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> + <tal:widget define="widget nocall:view/widgets/branch_owner">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> +
> + <tal:widget define="widget nocall:view/widgets/repo_url">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> +
> + <tr>
> + <td>
> + <label tal:replace="structure view/rcs_type_bzr">
> + Bazaar, hosted externally
> + </label>
> + </td>
> + </tr>
> +
> + <tr>
> + <td>
> + <label tal:replace="structure view/rcs_type_git">
> + Git
> + </label>
> + </td>
> + </tr>
> +
> + <tr>
> + <td>
> + <label tal:replace="structure view/rcs_type_svn">
> + SVN
> + </label>
> + </td>
> + </tr>
> +
> + <tr>
> + <td>
> + <label tal:replace="structure view/rcs_type_cvs">
> + CVS
> + </label>
> + <table class="subordinate">
> + <tal:widget define="widget nocall:view/widgets/cvs_module">
> + <metal:block use-macro="context/@@launchpad_form/widget_row" />
> + </tal:widget>
> + </table>
> + </td>
> + </tr>
> + </table>
> + </td>
> + </tr>
> + </table>
> + </div>
> + <input tal:replace="structure view/rcs_type_emptymarker" />
> + </div>
> +
> + </metal:formbody>
> + </div>
> +
> + <script type="text/javascript">
> + LPJS.use('lp.code.productseries_setbranch', function(Y) {
> + Y.on('domready', Y.lp.code.productseries_setbranch.setup);
> + });
> + </script>
> +
> + </div>
> + </body>
> +</html>
>
--
https://code.launchpad.net/~blr/launchpad/ui-project-setbranch/+merge/259069
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
