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/08 17:53:59 UTC
[GitHub] [trafficcontrol] rimashah25 opened a new pull request, #6948: Added layered profiles logic to TPv1
rimashah25 opened a new pull request, #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
Closes: https://github.com/apache/trafficcontrol/issues/6833
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Portal
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Add/Edit/Delete multiple profiles using Servers in TP. Confirm it matches the below acceptance criteria
- The layout for profile field is similar to interface field i.e. it will have a `+` button next to profile drop-down box.
- For selecting additional profiles, ensure that already selected profile are not available to select unless the priority order changes and the profile is removed.
- Also, if a profile is selected, have a delete button exists if there is more than one profile associated with a server.
**Note**: If you don't select a specific CDN, then all profiles (belonging to every CDN) will be seen in the drop down list.
Also, notice the difference in server table. For servers with multiple profiles, there will be an asterisk (*) next to first profile in the list. Hover tool tip over the profile and get the list of profiles as comma separated value.
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
## PR submission checklist
- [ ] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [ ] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
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.
-->
--
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
[GitHub] [trafficcontrol] github-code-scanning[bot] commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r920289278
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,15 @@
<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 to the server" ng-click="addProfile()"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="activeProfile-{{$index}}" name="activeProfile-{{$index}}" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required></select>
+ <small class="input-error" ng-show="hasPropertyError(serverForm['activeProfile-'+$index], 'required')">Required</small>
+ <a ng-href="{{getProfileID(server.profileNames[$index])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a>
Review Comment:
## Potentially unsafe external link
External links without noopener/noreferrer are a potential security risk.
[Show more details](https://github.com/apache/trafficcontrol/security/code-scanning/219)
--
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
[GitHub] [trafficcontrol] github-code-scanning[bot] commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r917032268
##########
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:
## Potentially unsafe external link
External links without noopener/noreferrer are a potential security risk.
[Show more details](https://github.com/apache/trafficcontrol/security/code-scanning/218)
--
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
[GitHub] [trafficcontrol] ocket8888 merged pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r920308380
##########
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:
Thanks for the assist, @ocket8888
--
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
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r918132336
##########
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
--
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
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r920738241
##########
traffic_portal/app/src/common/modules/form/server/form.server.tpl.html:
##########
@@ -110,14 +110,15 @@
<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 to the server" ng-click="addProfile()"><i class="fa fa-plus"></i></button></legend>
+ <fieldset ng-repeat="profile in server.profileNames track by $index">
+ <select id="activeProfile-{{$index}}" name="activeProfile-{{$index}}" class="form-control" ng-model="server.profileNames[$index]" ng-options="profile.name as profile.name for profile in profiles|excludeFilter:profile:server.profileNames" required></select>
+ <small class="input-error" ng-show="hasPropertyError(serverForm['activeProfile-'+$index], 'required')">Required</small>
+ <a ng-href="{{getProfileID(server.profileNames[$index])}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a>
Review Comment:
This link is always numeric, therefore always relative and never external.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r920527516
##########
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:
I am going to let it be as a function that returns. But I did remove the need to make it global.
--
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
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r920308044
##########
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:
Thanks for the assist, @ocket8888
##########
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:
Thanks for the assist @ocket8888
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #6948: Added layered profiles logic to TPv1
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on code in PR #6948:
URL: https://github.com/apache/trafficcontrol/pull/6948#discussion_r920528860
##########
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:
Understood. I saw someone do it on stack overflow, so used the same without understanding. Thank you for the explanation.
--
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