ocket8888 commented on code in PR #7480:
URL: https://github.com/apache/trafficcontrol/pull/7480#discussion_r1190316976
##########
experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.html:
##########
@@ -34,9 +34,14 @@
</mat-form-field>
<mat-form-field>
<mat-label>Secure</mat-label>
- <mat-select
name="secure"[(ngModel)]="parameter.secure" required>
- <mat-option *ngFor="let s of secure"
[value]="s.value">{{s.label}}</mat-option>
- </mat-select>
+ <input matInput type="boolean" name="secure"
required [(ngModel)]="parameter.secure" />
Review Comment:
`boolean` is not a valid value for the `type` attribute of an `input`. This
input shouldn't exist.
##########
experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.html:
##########
@@ -34,9 +34,14 @@
</mat-form-field>
<mat-form-field>
<mat-label>Secure</mat-label>
- <mat-select
name="secure"[(ngModel)]="parameter.secure" required>
- <mat-option *ngFor="let s of secure"
[value]="s.value">{{s.label}}</mat-option>
- </mat-select>
+ <input matInput type="boolean" name="secure"
required [(ngModel)]="parameter.secure" />
+ <mat-card-header matSuffix>
Review Comment:
`mat-card-header` belongs in the header of a `mat-card`, not inside of a
`mat-form-field`.
##########
experimental/traffic-portal/src/app/core/profiles/profile-table/profile-table.component.ts:
##########
@@ -115,11 +115,11 @@ export class ProfileTableComponent implements OnInit {
}
}
);
- const ID = this.route.snapshot.paramMap.get("id");
- if (ID === null) {
+ const hasParameter =
this.route.snapshot.queryParamMap.get("hasParameter");
+ if (hasParameter == null) {
Review Comment:
This is an interesting corner case. Apparently, the Angular default linter
settings don't use the default `eqeqeq` rule, and allow you to use either style
of comparison with `null` and only with `null`. _Technically_, there is a case
where this would fail to produce the same result as `===`, but it's not
possible given the type of `hasParameter` (specifically `null == undefined` is
`true` but `null === undefined` is `false`). So, since the linter doesn't care,
neither do I, but I'm making a mental note to correct that exclusion.
##########
experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.spec.ts:
##########
@@ -165,4 +165,29 @@ describe("ParametersTableComponent", () => {
expect(item.fragment).toBeUndefined();
expect(item.newTab).toBeTrue();
});
+
+ it("generates 'View Profiles' context menu item href", () => {
+ const item = component.contextMenuItems.find(i => i.name ===
"View Profiles");
+ if (!item) {
+ return fail("missing 'View Profiles' context menu
item");
+ }
+ if (isAction(item)) {
+ return fail("expected a link, not an action");
+ }
+ if (!item.href) {
+ return fail("missing 'href' property");
+ }
+ if (typeof(item.href) !== "string") {
+ return fail("'View Profiles' context menu item should
use a static string to determine href, instead uses a function");
+ }
+ expect(item.href).toBe("/core/profiles");
+ if (typeof(item.queryParams) !== "function") {
+ return fail(
+ `'View Profiles' context menu item should use a
function to determine query params, instead uses: ${item.queryParams}`
+ );
+ }
+ expect(item.queryParams(testParameter)).toEqual({hasParameter:
testParameter.id});
+ expect(item.fragment).toBeUndefined();
+ expect(item.newTab).toBeTrue();
Review Comment:
Should this be opening in a new tab? I don't believe other, similar links do
that.
##########
experimental/traffic-portal/src/app/core/parameters/table/parameters-table.component.ts:
##########
@@ -53,19 +55,24 @@ export class ParametersTableComponent implements OnInit {
},
{
field: "profiles",
- headerName: "Profiles"
+ headerName: "Profiles",
+ valueFormatter: ({data}: {data: ResponseParameter}):
string => `${data.profiles}`
Review Comment:
Better, but this won't put any spaces between elements. It's a bit harder to
read that way, and I'm not sure how a screen reader would handle it but I'd
wager it's not pretty.
E.g.
##########
experimental/traffic-portal/src/app/api/testing/profile.service.ts:
##########
@@ -205,4 +205,76 @@ export class ProfileService {
return this.profiles.splice(index, 1)[0];
}
+ private lastParamID = 20;
+ private readonly parameters = [
+ {
+ configFile: "cfg.txt",
+ id: 1,
+ lastUpdated: new Date(),
+ name: "param1",
+ profiles: [],
+ secure: false,
+ value: "10"
+ }
+ ];
+
+ public async getParameters(idOrName: number | string):
Promise<ResponseParameter>;
+ public async getParameters(): Promise<Array<ResponseParameter>>;
+ /**
+ * Gets one or all Parameters from Traffic Ops
+ *
+ * @param idOrName Either the integral, unique identifier (number) or
name (string) of a single parameter to be returned.
+ * @returns The requested parameter(s).
+ */
+ public async getParameters(idOrName?: number | string):
Promise<ResponseParameter | Array<ResponseParameter>> {
+ if (idOrName !== undefined) {
+ let parameter;
+ switch (typeof idOrName) {
+ case "string":
+ parameter =
this.parameters.filter(t=>t.name === idOrName)[0];
+ break;
+ case "number":
+ parameter =
this.parameters.filter(t=>t.id === idOrName)[0];
+ }
+ if (!parameter) {
+ throw new Error(`no such Parameter:
${idOrName}`);
+ }
+ return parameter;
+ }
+ return this.parameters;
+ }
+
+ /**
+ * Deletes an existing parameter.
+ *
+ * @param id Id of the parameter to delete.
+ * @returns The deleted parameter.
+ */
+ public async deleteParameter(id: number): Promise<ResponseParameter> {
+ const index = this.parameters.findIndex(t => t.id === id);
+ if (index === -1) {
+ throw new Error(`no such Parameter: ${id}`);
+ }
+ return this.parameters.splice(index, 1)[0];
+ }
+
+ /**
+ * Creates a new parameter.
+ *
+ * @param parameter The parameter to create.
+ * @returns The created parameter.
+ */
+ public async createParameter(parameter: RequestParameter):
Promise<ResponseParameter> {
+ const t = {
+ ...parameter,
+ configFile: "cfg.txt",
+ id: ++this.lastParamID,
+ lastUpdated: new Date(),
+ profiles: [],
+ secure: false,
+ value: "100"
Review Comment:
you should not have to change the `configFile`, `secure`, or `value`
properties.
##########
experimental/traffic-portal/src/app/core/parameters/detail/parameter-detail.component.ts:
##########
@@ -105,11 +106,23 @@ export class ParameterDetailComponent implements OnInit {
e.preventDefault();
e.stopPropagation();
if(this.new) {
- this.parameter = await
this.parameterService.createParameter(this.parameter);
+ this.parameter = await
this.profileService.createParameter(this.parameter);
this.new = false;
} else {
- this.parameter = await
this.parameterService.updateParameter(this.parameter);
+ this.parameter = await
this.profileService.updateParameter(this.parameter);
}
}
+ /**
+ * Sets the value of a parameter, corresponding to the value of the
toggle bar
+ *
+ * @param e MatSlideToggleChange event.
+ */
+ public async setValue(e: MatSlideToggleChange): Promise<void> {
Review Comment:
You're working too hard with this. If you do an `ngModel` 2-way binding to
the parameter's `secure` property, you're done, with no extra ternaries or
controller methods required.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
