You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/07/11 16:36:38 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
ocket8888 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r918073961
##########
traffic_portal/app/src/common/filters/ExcludeFilter.js:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var ExcludeFilter = function() {
Review Comment:
This doesn't need to be made global, could just be `function ExcludeFilter() {...`
It would also probably be more readable - though it has no effect on behavior - if `ExcludeFilter` was just the actual filter and not a function that returns it. Then the export line would just be `module.exports = ()=>ExcludeFilter` instead. Up to you, but it's what I would do.
##########
traffic_portal/app/src/common/filters/ExcludeFilter.js:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var ExcludeFilter = function() {
+ return function(list,currentModel,selected){
+ var selectedLength = selected.length;
+ var out = [];
+ angular.forEach(list,function(listItem){
+ var add = true;
Review Comment:
Should use `let` rather than `var`; this will add `add` as a property to `this`, which is manipulated by `angular.forEach` (although you're not actually changing the reference with this invocation).
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,17 @@
<small ng-show="server.typeId"><a ng-href="/#!/types/{{server.typeId}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
</div>
- <label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
- <div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[0]" ng-options="profile.name as profile.name for profile in profiles" required>
- <option hidden selected disabled value="">Select...</option>
- </select>
- <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
- <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
- </div>
+ <fieldset>
+ <legend>Profiles<button name="addProfileBtn" class="btn btn-primary right-button btn-xs" type="button" title="add a new profile" ng-click="addProfile(server.cdnId)"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required>
+ <option hidden selected disabled value="">Select...</option>
+ </select>
+ <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
Review Comment:
This error message will never be shown, as the `select` control is never "falsey" despite being `required`. If the controller is changed to instead push `null` into `profileNames`, then this will trigger. However, it will trigger on *all* of the `profileNames` controls whenever it triggers on *any* of them, because they all use the same `name` - `profile` - and this checks `serverForm.profile`. See attached screenshot.
Each `select` should independently check its own `required` status and display errors only on the fields that have the error.
##########
traffic_portal/app/src/common/filters/ExcludeFilter.js:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var ExcludeFilter = function() {
+ return function(list,currentModel,selected){
+ var selectedLength = selected.length;
+ var out = [];
+ angular.forEach(list,function(listItem){
+ var add = true;
+ for (var index = 0; index < selectedLength; index++) {
Review Comment:
Should use `let` rather than `var` to avoid hoisting the declaration to an unintended scope.
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,17 @@
<small ng-show="server.typeId"><a ng-href="/#!/types/{{server.typeId}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
</div>
- <label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
- <div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[0]" ng-options="profile.name as profile.name for profile in profiles" required>
- <option hidden selected disabled value="">Select...</option>
- </select>
- <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
- <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
- </div>
+ <fieldset>
+ <legend>Profiles<button name="addProfileBtn" class="btn btn-primary right-button btn-xs" type="button" title="add a new profile" ng-click="addProfile(server.cdnId)"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required>
Review Comment:
Even though this is `required`, because it's never set to a "falsey" value the user can click the submit button without entering a value, which gives back the cryptic error-level Alert
> json: cannot unmarshal object into Go struct field ServerV40.profileNames of type string
b
##########
traffic_portal/app/src/common/filters/ExcludeFilter.js:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var ExcludeFilter = function() {
+ return function(list,currentModel,selected){
+ var selectedLength = selected.length;
+ var out = [];
+ angular.forEach(list,function(listItem){
Review Comment:
We don't need `angular.forEach` for this. It can be `for (const listItem of list) {...` to just loop directly, or `list.forEach(listItem => {...` to do something very similar to `angular.forEach`, or even just `for (let i = 0; i < list.length; ++i) { const listItem = list[i]; ...` (though I wouldn't recommend that). `angular.forEach`'s purpose is to execute the provided function with a custom binding for `this` - which is confusing and should never be done, especially when the collection isn't being modified like here.
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,17 @@
<small ng-show="server.typeId"><a ng-href="/#!/types/{{server.typeId}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
</div>
- <label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
- <div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[0]" ng-options="profile.name as profile.name for profile in profiles" required>
- <option hidden selected disabled value="">Select...</option>
- </select>
- <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
- <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
- </div>
+ <fieldset>
+ <legend>Profiles<button name="addProfileBtn" class="btn btn-primary right-button btn-xs" type="button" title="add a new profile" ng-click="addProfile(server.cdnId)"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required>
+ <option hidden selected disabled value="">Select...</option>
Review Comment:
Because the value being set is never "falsey" - as above it's being set to an object - this option is unused and never shown. If the controller is changed to instead push `null` into the `profileNames` array, this option will be shown by default and not shown in the dropdown, always un-select-able after the user makes a selection, which is its intended purpose.
##########
traffic_portal/app/src/common/filters/ExcludeFilter.js:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var ExcludeFilter = function() {
+ return function(list,currentModel,selected){
+ var selectedLength = selected.length;
+ var out = [];
Review Comment:
`out` and `selectedLength` are never reassigned, could be `const`.
##########
traffic_portal/app/src/common/filters/ExcludeFilter.js:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+var ExcludeFilter = function() {
+ return function(list,currentModel,selected){
+ var selectedLength = selected.length;
+ var out = [];
+ angular.forEach(list,function(listItem){
+ var add = true;
+ for (var index = 0; index < selectedLength; index++) {
+ if(selected[index] !== currentModel && selected[index] === listItem.name){
+ add = false;
+ break;
+ }
+ }
+
+ if(add){
+ out.push(listItem);
+ }
+
+ });
+
+ return out;
+ };
Review Comment:
So, as I understand it, this function seems to be doing some extra work, by re-iterating the input `selected` list for `O(n²)` complexity on every digest cycle. The filtering being done seems to be "all of the items in `list` that aren't `currentModel` and aren't in the `selected` array". Given that the comparisons are being done using `===`, `selected` is being functionally treated as a set, but is iterated like a list for every profile. So if you instead just create a set of undesirable items and then filter by what's in that list, you can make the filter `O(2n)` instead, like:
```
function filter(list, currentModel, selected) {
const filterOut = new Set(selected.concat(currentModel));
return list.filter(listItem => !filterOut.has(listItem));
}
```
which is also a bit easier to understand at a glance.
##########
traffic_portal/app/src/common/modules/table/servers/TableServersController.js:
##########
@@ -163,7 +163,16 @@ var TableServersController = function(tableName, servers, filter, $scope, $state
{
headerName: "Profile",
field: "profileName",
- hide: false
+ hide: false,
+ valueGetter: function(params) {
+ if (params.data.profileNames.length > 1) {
+ return params.data.profileName + ' *';
+ }
+ return params.data.profileName;
+ },
+ tooltipValueGetter: function(params) {
+ return params.data.profileNames.toString()
Review Comment:
Nit but this would have no spacing; e.g. if the server in question has Profiles `["test", "quest", "testquest"]` the tooltip will read "test,quest,testquest". If joined with a space instead, like `return params.data.profileNames.join(", ");` it would be easier to read: "test, quest, testquest".
##########
traffic_portal/app/src/common/modules/form/server/FormServerController.js:
##########
@@ -69,6 +69,24 @@ var FormServerController = function(server, $scope, $location, $state, $uibModal
}
};
+ $scope.addProfile = function(cdnId) {
+ $scope.serverForm.$setDirty();
+ const newProfile = {
+ profileNames: getProfiles(cdnId)
+ };
Review Comment:
I'm not sure why this is being done. The value `{ profileNames: getProfiles(cdnId) }` is not a valid Profile name, it's an object. Also the results of that HTTP request being sent every time the user clicks the `+` button don't appear to be used anywhere. This should just push some invalid value - ideally `null`, more on that below - into the server's `profileNames` array.
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,17 @@
<small ng-show="server.typeId"><a ng-href="/#!/types/{{server.typeId}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
</div>
- <label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
- <div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[0]" ng-options="profile.name as profile.name for profile in profiles" required>
- <option hidden selected disabled value="">Select...</option>
- </select>
- <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
- <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
- </div>
+ <fieldset>
+ <legend>Profiles<button name="addProfileBtn" class="btn btn-primary right-button btn-xs" type="button" title="add a new profile" ng-click="addProfile(server.cdnId)"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required>
+ <option hidden selected disabled value="">Select...</option>
+ </select>
+ <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
+ <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
+ <button class="btn btn-danger right-button btn-xs" type="button" title="delete this profile" ng-if="server.profileNames.length>1" ng-click="deleteProfile(profile)"><i class="fa fa-minus"></i></button>
Review Comment:
This `title` is just a bit misleading. Deleting a Profile is something you can do and it's very different than what this button does. It should instead say something like "remove this Profile from the server".
Also, since you have a reference to `$index`, you could pass that instead of just the Profile name, so that the `deleteProfile` function doesn't need to iterate the server's Profile list to figure out what that index is.
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,17 @@
<small ng-show="server.typeId"><a ng-href="/#!/types/{{server.typeId}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
</div>
- <label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
- <div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[0]" ng-options="profile.name as profile.name for profile in profiles" required>
- <option hidden selected disabled value="">Select...</option>
- </select>
- <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
- <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
- </div>
+ <fieldset>
+ <legend>Profiles<button name="addProfileBtn" class="btn btn-primary right-button btn-xs" type="button" title="add a new profile" ng-click="addProfile(server.cdnId)"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required>
+ <option hidden selected disabled value="">Select...</option>
+ </select>
+ <small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
+ <small ng-show="server.profileNames.length>0"><a ng-href="{{getProfileID(server.profileNames[0])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
Review Comment:
This button cannot possibly rendered if `server.profileNames.length` isn't `>0`, because the `fieldset` containing it won't exist. It should instead check that the particular `profileNames` element with which it's associated is "truthy" (once the controller is updated to only use valid "truthy" values). Also, this always opens the details for the server's first Profile, no matter which Profile is above the button the user clicks on. It should show the details for the selected Profile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org