feat: make configurable action buttons shared component #1977
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to SWAP-4889-configurable-action-buttons
…b.com/SciCatProject/frontend into SWAP-4889-configurable-action-buttons
|
Hi, two points I noticed while testing:
In general, would it make sense to unify the different methods a little bit? Building the info as an object in a shared way and then transforming it into json/the form etc? I tested it when the PR was at |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:255` </location>
<code_context>
- this.prepare_disabled_condition();
-
- const expr = this.disabled_condition;
- const fn = new Function("ctx", `with (ctx) { return (${expr}); }`);
-
- return fn({
</code_context>
<issue_to_address>
**🚨 issue (security):** Use of 'new Function' and 'with' may introduce security and maintainability risks.
Consider replacing dynamic code execution with a safer, more maintainable solution, such as an explicit expression parser or a whitelist of permitted operations.
</issue_to_address>
### Comment 2
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:318` </location>
<code_context>
- }
-
- type_form() {
- if (this.form !== null) {
- document.body.removeChild(this.form);
- }
</code_context>
<issue_to_address>
**suggestion:** Direct DOM manipulation for form creation may cause issues in Angular lifecycle and testing.
This approach bypasses Angular's templating and change detection, leading to unpredictable behavior and harder testing. Use Angular's form APIs or encapsulate DOM operations in a service instead.
Suggested implementation:
```typescript
formGroup: FormGroup;
constructor(private fb: FormBuilder) {}
type_form() {
// Example: create a form group using Angular's FormBuilder
this.formGroup = this.fb.group({
field1: [''],
field2: ['']
// Add more fields as needed
});
}
```
1. You will need to import `FormBuilder` and `FormGroup` at the top of your file:
```typescript
import { FormBuilder, FormGroup } from '@angular/forms';
```
2. Update your template (`configurable-action.component.html`) to render the form using Angular's form directives, e.g.:
```html
<form [formGroup]="formGroup">
<input formControlName="field1" />
<input formControlName="field2" />
<!-- Add more fields as needed -->
</form>
```
3. If you need to dynamically create forms based on some configuration, adjust the `type_form()` method to build the form group accordingly.
</issue_to_address>
### Comment 3
<location> `src/app/shared/modules/configurable-actions/configurable-actions.component.ts:29-33` </location>
<code_context>
- }
-
- get sortedActionsConfig(): ActionConfig[] {
- this._sortedActionsConfig = this.actionsConfig;
- this._sortedActionsConfig.sort((a: ActionConfig, b: ActionConfig) =>
- a.order && b.order ? a.order - b.order : 0,
- );
- return this._sortedActionsConfig;
- }
-}
</code_context>
<issue_to_address>
**issue (bug_risk):** Sorting mutates the input array, which may have unintended side effects.
Make a copy of 'this.actionsConfig' before sorting to prevent unintended side effects on the original array.
</issue_to_address>
### Comment 4
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:482` </location>
<code_context>
+ }
+
+ type_link() {
+ window.open(this.actionConfig.url, this.actionConfig.target || "_self");
+ }
+}
</code_context>
<issue_to_address>
**suggestion:** No variable interpolation for URLs in 'type_link' may limit flexibility.
Consider implementing variable interpolation for 'type_link' URLs to support dynamic values, similar to other action types.
Suggested implementation:
```typescript
type_link() {
// Interpolate variables in the URL before opening
const interpolatedUrl = this.interpolateVariables(this.actionConfig.url, this.actionConfig.variables || {});
window.open(interpolatedUrl, this.actionConfig.target || "_self");
}
```
If `interpolateVariables` is not already implemented or available in this class/component, you will need to add or import it.
For example, you might need to add a method like:
```typescript
interpolateVariables(template: string, variables: {[key: string]: any}): string {
return template.replace(/\{\{(\w+)\}\}/g, (_, key) => variables[key] ?? '');
}
```
Or import it from a shared utility if it exists elsewhere in your codebase.
</issue_to_address>
### Comment 5
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:119` </location>
<code_context>
+ styleUrls: ["./configurable-action.component.scss"],
+ standalone: false,
+})
+export class ConfigurableActionComponent implements OnInit, OnChanges {
+ @Input({ required: true }) actionConfig: ActionConfig;
+ @Input({ required: true }) actionItems: ActionItems;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the component by extracting selector parsing, condition evaluation, and action execution logic into dedicated services to simplify the component and reduce its complexity.
Here are a few orthogonal refactorings you can apply incrementally to dramatically reduce the size and cyclomatic‐complexity of your component, without changing any behavior:
1) Extract all selector‐parsing logic into its own service
2) Extract condition‐building & evaluation into a “ConditionService”
3) Extract all “type_…” methods into an “ActionExecutorService”
4) Wire them back into your component via DI and shrink your switch/`new Function` calls
----
### 1) New `ActionSelectorService`
```ts
// action-selector.service.ts
import { Injectable } from "@angular/core";
import { ActionItems } from "./configurable-action.interfaces";
@Injectable({ providedIn: 'root' })
export class ActionSelectorService {
private keywordMap = { /* copy your keywordMap here */ };
processSelector(items: ActionItems, selector: string): any {
for (const [pattern, fn] of Object.entries(this.keywordMap)) {
const m = selector.match(new RegExp(pattern));
if (m) { return fn(m); }
}
return selector;
}
}
```
### 2) New `ConditionService`
```ts
// condition.service.ts
import { Injectable } from "@angular/core";
@Injectable({ providedIn: 'root' })
export class ConditionService {
buildDisabled(expr: string, variables: any, maxSize: number, owner: boolean): () => boolean {
// inline your prepare_action_condition + new Function here
const prepared = expr
.replace(/\@(\w+)/g, (_, v) => `variables.${v}`)
.replace(/\#Length\(@(\w+)\)/g, (_, v) => `variables.${v}.length`)
// … etc
const fn = new Function('variables','maxSize','owner', `return (${prepared});`);
return () => fn(variables, maxSize, owner);
}
buildHidden(expr: string, /* … */): () => boolean { /* same idea */ }
}
```
### 3) New `ActionExecutorService`
```ts
// action-executor.service.ts
import { Injectable } from "@angular/core";
import { ActionConfig } from "./configurable-action.interfaces";
@Injectable({ providedIn: 'root' })
export class ActionExecutorService {
constructor(/* inject AuthService, AppConfigService, MatSnackBar… */){}
run(config: ActionConfig, vars: any, jwt: string): boolean {
const fn = this.map[config.type || 'form'];
return fn.call(this, config, vars, jwt);
}
private map = {
form: this.doForm,
xhr: this.doXhr,
link: this.doLink,
'json-download': this.doJsonDownload,
};
private doForm(cfg: ActionConfig, vars: any) {
// copy your type_form() body here, calling a small helper to generate <input>
}
private doXhr(...) { /* … */ }
private doLink(...) { /* … */ }
private doJsonDownload(...) { /* … */ }
}
```
### 4) Shrink your component down to wiring only
```ts
@Component({ /* … */ })
export class ConfigurableActionComponent
implements OnInit, OnChanges {
@Input() actionConfig: ActionConfig;
@Input() actionItems: ActionItems;
variables: Record<string, any> = {};
constructor(
private sel: ActionSelectorService,
private cond: ConditionService,
private exec: ActionExecutorService,
private store: Store, /* … */
) {}
ngOnInit() { this.updateStatus(); /* … */ }
ngOnChanges() { this.updateStatus(); }
private updateStatus() {
Object.entries(this.actionConfig.variables || {})
.forEach(([k, selStr]) => {
this.variables[k] = this.sel.processSelector(this.actionItems, selStr as string);
});
}
get disabled() {
const fn = this.cond.buildDisabled(
this.actionConfig.enabled || this.actionConfig.disabled || 'false',
this.variables,
/* maxSize */, /* owner */
);
return fn();
}
perform_action() {
return this.exec.run(this.actionConfig, this.variables, /* jwt */);
}
}
```
With these extractions:
- The component no longer needs 500 lines of parsing, regexps or DOM hacks.
- You vastly reduce cyclomatic–complexity by moving each concern into its own service.
- You can unit-test each service in isolation.
</issue_to_address>
### Comment 6
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.spec.ts:127-134` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 7
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.spec.ts:568-598` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 8
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.spec.ts:608-622` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 9
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:103-105` </location>
<code_context>
const res = fn(match);
return res;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return fn(match);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 10
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:255` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Never use the `Function` constructor. ([`no-new-function`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/no-new-function))
<details><summary>Explanation</summary>Creating a function using the `Function` constructor evaluates a string similarly to `eval()` which may open
up vulnerabilities.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#functions--constructor)
</details>
</issue_to_address>
### Comment 11
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:257` </location>
<code_context>
const context = this.context;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {context} = this;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 12
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:259-261` </location>
<code_context>
const res = fn(context);
return res;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return fn(context);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 13
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:269` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Never use the `Function` constructor. ([`no-new-function`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/no-new-function))
<details><summary>Explanation</summary>Creating a function using the `Function` constructor evaluates a string similarly to `eval()` which may open
up vulnerabilities.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#functions--constructor)
</details>
</issue_to_address>
### Comment 14
<location> `src/app/shared/modules/configurable-actions/configurable-action.component.ts:363-382` </location>
<code_context>
const readyPayload = payload.replace(
/\{\{\s*([@#]\w+(\[\])?)\s*\}\}/g,
(_, variableName) => {
if (variableName.endsWith("[]")) {
const variableNameClean = variableName.slice(0, -2);
const value = this.get_value_from_definition(variableNameClean);
const iteratable = !value
? []
: Array.isArray(value)
? value
: [value];
return JSON.stringify(iteratable);
} else {
return this.get_value_from_definition(variableName);
}
},
);
return readyPayload;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return payload.replace(
/\{\{\s*([@#]\w+(\[\])?)\s*\}\}/g,
(_, variableName) => {
if (variableName.endsWith("[]")) {
const variableNameClean = variableName.slice(0, -2);
const value = this.get_value_from_definition(variableNameClean);
const iteratable = !value
? []
: Array.isArray(value)
? value
: [value];
return JSON.stringify(iteratable);
} else {
return this.get_value_from_definition(variableName);
}
},
);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 15
<location> `src/app/shared/modules/configurable-actions/configurable-actions.component.spec.ts:74` </location>
<code_context>
const sortedActionsConfig = component.sortedActionsConfig;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {sortedActionsConfig} = component;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 16
<location> `src/app/shared/modules/filters/condition-filter.component.ts:17` </location>
<code_context>
const condition = this.condition;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {condition} = this;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 17
<location> `src/app/shared/modules/filters/group-filter.component.ts:63` </location>
<code_context>
const value = (<HTMLInputElement>event.target).value;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {value} = <HTMLInputElement>event.target;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 18
<location> `src/app/shared/modules/filters/keyword-filter.component.ts:77` </location>
<code_context>
const value = (<HTMLInputElement>event.target).value;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {value} = <HTMLInputElement>event.target;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 19
<location> `src/app/shared/modules/filters/location-filter.component.ts:75` </location>
<code_context>
const value = (<HTMLInputElement>event.target).value;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {value} = <HTMLInputElement>event.target;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 20
<location> `src/app/shared/modules/filters/pid-filter.component.ts:43` </location>
<code_context>
const condition = !pid ? "" : this.buildPidTermsCondition(pid);
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert ternary operator to remove negation ([`invert-ternary`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/invert-ternary))
```suggestion
const condition = pid ? this.buildPidTermsCondition(pid) : "";
```
<br/><details><summary>Explanation</summary>Negated conditions are more difficult to read than positive ones, so it is best
to avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
</details>
</issue_to_address>
### Comment 21
<location> `src/app/shared/modules/filters/type-filter.component.ts:62` </location>
<code_context>
const value = (<HTMLInputElement>event.target).value;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {value} = <HTMLInputElement>event.target;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| const expr = this.disabled_condition; | ||
|
|
||
| const fn = new Function("ctx", `with (ctx) { return (${expr}); }`); |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Use of 'new Function' and 'with' may introduce security and maintainability risks.
Consider replacing dynamic code execution with a safer, more maintainable solution, such as an explicit expression parser or a whitelist of permitted operations.
| } | ||
|
|
||
| type_form() { | ||
| if (this.form !== null) { |
Contributor
There was a problem hiding this comment.
suggestion: Direct DOM manipulation for form creation may cause issues in Angular lifecycle and testing.
This approach bypasses Angular's templating and change detection, leading to unpredictable behavior and harder testing. Use Angular's form APIs or encapsulate DOM operations in a service instead.
Suggested implementation:
formGroup: FormGroup;
constructor(private fb: FormBuilder) {}
type_form() {
// Example: create a form group using Angular's FormBuilder
this.formGroup = this.fb.group({
field1: [''],
field2: ['']
// Add more fields as needed
});
}- You will need to import
FormBuilderandFormGroupat the top of your file:import { FormBuilder, FormGroup } from '@angular/forms';
- Update your template (
configurable-action.component.html) to render the form using Angular's form directives, e.g.:<form [formGroup]="formGroup"> <input formControlName="field1" /> <input formControlName="field2" /> <!-- Add more fields as needed --> </form>
- If you need to dynamically create forms based on some configuration, adjust the
type_form()method to build the form group accordingly.
Comment on lines
29
to
33
| this._sortedActionsConfig = this.actionsConfig; | ||
| this._sortedActionsConfig.sort((a: ActionConfig, b: ActionConfig) => | ||
| a.order && b.order ? a.order - b.order : 0, | ||
| ); | ||
| return this._sortedActionsConfig; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Sorting mutates the input array, which may have unintended side effects.
Make a copy of 'this.actionsConfig' before sorting to prevent unintended side effects on the original array.
Comment on lines
+127
to
+134
| function createComponent(componentActionConfig, componentsActionItems) { | ||
| fixture = TestBed.createComponent(ConfigurableActionComponent); | ||
| component = fixture.componentInstance; | ||
| component.actionConfig = componentActionConfig; | ||
| component.actionItems = componentsActionItems; | ||
| fixture.detectChanges(); | ||
| return component; | ||
| } |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
Comment on lines
+568
to
+598
| function selectTestCase(testCase: TestCase) { | ||
| const userProfile = mockUserProfiles[testCase.user] || {}; | ||
|
|
||
| store.overrideSelector(selectProfile, userProfile); | ||
| store.refreshState(); | ||
|
|
||
| const currentActionConfig = mockActionsConfig.filter( | ||
| (a) => a.id == testCase.action, | ||
| )[0]; | ||
|
|
||
| switch (testCase.limit) { | ||
| case maxSizeType.higher: | ||
| mockAppConfigService.appConfig.maxDirectDownloadSize = | ||
| higherMaxFileSizeLimit; | ||
| break; | ||
| case maxSizeType.lower: | ||
| default: | ||
| mockAppConfigService.appConfig.maxDirectDownloadSize = | ||
| lowerMaxFileSizeLimit; | ||
| break; | ||
| } | ||
|
|
||
| const published: boolean | string = testCase.published || false; | ||
| if (typeof published === "boolean") { | ||
| testCase.actionItems.datasets.forEach((dataset) => { | ||
| dataset.isPublished = published; | ||
| }); | ||
| } | ||
|
|
||
| createComponent(currentActionConfig, testCase.actionItems); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| } | ||
|
|
||
| onGroupInput(event: any) { | ||
| const value = (<HTMLInputElement>event.target).value; |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const value = (<HTMLInputElement>event.target).value; | |
| const {value} = <HTMLInputElement>event.target; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| } | ||
|
|
||
| onKeywordInput(event: any) { | ||
| const value = (<HTMLInputElement>event.target).value; |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const value = (<HTMLInputElement>event.target).value; | |
| const {value} = <HTMLInputElement>event.target; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| } | ||
|
|
||
| onLocationInput(event: any) { | ||
| const value = (<HTMLInputElement>event.target).value; |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const value = (<HTMLInputElement>event.target).value; | |
| const {value} = <HTMLInputElement>event.target; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| this.subscription = this.pidSubject | ||
| .pipe(debounceTime(500)) | ||
| .subscribe((pid) => { | ||
| const condition = !pid ? "" : this.buildPidTermsCondition(pid); |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Invert ternary operator to remove negation (invert-ternary)
Suggested change
| const condition = !pid ? "" : this.buildPidTermsCondition(pid); | |
| const condition = pid ? this.buildPidTermsCondition(pid) : ""; |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
| this.label = getFilterLabel(filters, this.componentName, this.label); | ||
| } | ||
| onTypeInput(event: any) { | ||
| const value = (<HTMLInputElement>event.target).value; |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const value = (<HTMLInputElement>event.target).value; | |
| const {value} = <HTMLInputElement>event.target; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors the datafiles action as a configurable components that can be used in other pages of the app.
The configuration has been updated to make the configurable actions more flexible.
Motivation
Up to this point, SciCat had only configurable actions for the data files associated to each dataset.
In the last year, it have become clear that we need to expand this concept also to dataset as a whole, the datasets list, all the way to the selection.
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
Backend version
Summary by Sourcery
Extract datafiles action buttons into a shared, configurable component and remove the legacy dataset-specific implementations
New Features:
Enhancements:
Tests:
Chores:
Summary by Sourcery
Introduce a reusable configurable action buttons component and migrate the existing datafiles actions to use it, generalize action settings in the app config, and clean up the legacy implementations. Also add a suite of shared dataset filter components and update tests accordingly.
New Features:
Enhancements:
Tests:
Chores: