Code review comment for ~bjornt/maas:peer-proxy-ui

Maria's design and preference is what we have, but I'll talk to her
tomorrow!

On Fri, Jul 21, 2017 at 1:45 AM Lee Trager <email address hidden> wrote:

> I really like splitting NTP and DNS into different sections, it makes the
> page much easier to read. I think we may want to use a drop down to select
> the proxy mode. The text field would be disabled for don't use and
> built-in. I'd ask Maria from design this before reworking incase I'm wrong.
>
> I poked around and can't figure out why MAAS.templates is currently broken
> and wasn't able to come up with anything, Blake might know.
>
> Diff comments:
>
> > diff --git
> a/src/maasserver/static/js/angular/directives/proxy_settings.js
> b/src/maasserver/static/js/angular/directives/proxy_settings.js
> > new file mode 100644
> > index 0000000..0850058
> > --- /dev/null
> > +++ b/src/maasserver/static/js/angular/directives/proxy_settings.js
> > @@ -0,0 +1,43 @@
> > +/* Copyright 2018 Canonical Ltd. This software is licensed under the
>
> Pulling code from the future? ;)
>
> > + * GNU Affero General Public License version 3 (see the file LICENSE).
> > + *
> > + * Proxy settings directive.
> > +*/
> > +
> > +angular.module('MAAS').directive('maasProxySettings', [
> > + '$sce', 'ConfigsManager', 'ManagerHelperService', 'JSONService',
> > + function($sce, ConfigsManager, ManagerHelperService, JSONService) {
> > + return {
> > + restrict: "E",
> > + scope: {},
> > + templateUrl: 'static/partials/proxy-settings.html',
> > + controller: function($scope, $rootScope, $element,
> $document) {
> > + $scope.loading = true;
> > + $scope.configManager = ConfigsManager;
> > + var managers = [ConfigsManager];
> > + ManagerHelperService.loadManagers(
> > + $scope, managers).then(function() {
> > + $scope.loading = false;
> > + $scope.httpProxy = ConfigsManager.getItemFromList(
> > + "http_proxy");
> > + $scope.enableHttpProxy =
> ConfigsManager.getItemFromList(
> > + "enable_http_proxy");
> > + $scope.usePeerProxy =
> ConfigsManager.getItemFromList(
> > + "use_peer_proxy");
> > + if ($scope.enableHttpProxy.value) {
> > + if ($scope.httpProxy.value) {
> > + if ($scope.usePeerProxy.value) {
> > + $scope.proxy_type = "peer-proxy";
> > + } else {
> > + $scope.proxy_type = "external-proxy";
> > + }
> > + } else {
> > + $scope.proxy_type = "builtin-proxy";
> > + }
> > + } else {
> > + $scope.proxy_type = "no-proxy";
> > + }
> > + });
> > + }
> > + };
> > + }]);
> > diff --git
> a/src/maasserver/static/js/angular/directives/tests/test_proxy_settings.js
> b/src/maasserver/static/js/angular/directives/tests/test_proxy_settings.js
> > new file mode 100644
> > index 0000000..e7ae24c
> > --- /dev/null
> > +++
> b/src/maasserver/static/js/angular/directives/tests/test_proxy_settings.js
> > @@ -0,0 +1,71 @@
> > +
> > +/* Copyright 2016 Canonical Ltd. This software is licensed under the
>
> Wrong year here as well.
>
> > + * GNU Affero General Public License version 3 (see the file LICENSE).
> > + *
> > + * Unit tests for MAAS proxy settings directive.
> > + */
> > +
> > +describe("maasProxySettings", function() {
> > +
> > + // Load the MAAS module.
> > + beforeEach(module("MAAS"));
> > + // Make the templates available.
> > + beforeEach(module("MAAS.templates"));
> > +
> > + // Get required angular pieces and create a new scope before each
> test.
> > + var $scope , $timeout, $compile, $q;
> > + beforeEach(inject(function($rootScope, $injector) {
> > + $scope = $rootScope.$new();
> > + $compile = $injector.get("$compile");
> > + }));
> > +
> > +
> > + // Return the compiled directive.
> > + function compileDirective(html) {
> > + html = "<div>" + html + "</div>";
> > + var directive = $compile(html)($scope);
> > + $scope.$digest();
> > + return directive.children(":first");
> > + }
> > +
> > + // Changes value on field.
> > + function changeFieldValue(field, val) {
> > + // Grab focus.
> > + field.triggerHandler("focus");
> > + $scope.$digest();
> > +
> > + // Set the new value.
> > + field.val(val);
> > + $scope.$digest();
> > +
> > + // Lose focus.
> > + field.triggerHandler("blur");
> > + $scope.$digest();
> > + }
> > +
> > + describe("no proxy", function() {
> > +
> > + var directive;
> > + beforeEach(function() {
> > + var html = [
> > + '<maas-proxy-settings />'
> > + ].join('');
> > + $scope.loading = false;
> > + $scope.httpProxy = {value: ""};
> > + $scope.enableHttpProxy = {value: false};
> > + $scope.usePeerProxy = {value: false};
> > + $scope.proxy_type = 'no-proxy';
> > + directive = compileDirective(html);
> > + });
> > +
> > + it("enable_http_proxy is False", function() {
> > + debugger;
> > + var inputField = angular.element(directive.find(
> > + "#id_proxy-enable_http_proxy"));
> > + // XXX: This test doesn't work. The directive renders the
> > + // default True insteads of False, since proxy_type is ''
> > + // for some reason.
> > + //expect(inputField.attr("value")).toBe("False");
> > + });
> > + });
> > +});
>
>
> --
> https://code.launchpad.net/~bjornt/maas/+git/maas/+merge/327591
> Your team MAAS Maintainers is requested to review the proposed merge of
> ~bjornt/maas:peer-proxy-ui into maas:master.
>
--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer