Skip to content

Conversation

@ghazwarhili
Copy link
Contributor

PR Summary

Shunt compensator modification dialog migration ts

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link
Contributor

@Mathieu-Deharbe Mathieu-Deharbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests ok. I didn't finish, juste some small remarks to be discussed when I come back. If someone else reviews this in the meantime you can merge without me.

Comment on lines 128 to 138
export const getCharacteristicsCreateFormDataFromSearchCopy = ({
bperSection,
bPerSection,
qAtNominalV,
sectionCount,
maximumSectionCount,
}: {
bPerSection: number;
qAtNominalV: number;
sectionCount: number;
maximumSectionCount: number;
}) => {
Copy link
Contributor

@Mathieu-Deharbe Mathieu-Deharbe Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the recommended format for gridsuite ? (to keep those grouped parameters as an objet)

Not like this : ?

export const getCharacteristicsCreateFormDataFromSearchCopy = (
    bPerSection: number;
    qAtNominalV: number;
    sectionCount: number;
    maximumSectionCount: number;
) => {

Is there a generic rule for the project @etiennehomer ?

Comment on lines +60 to +72
voltageLevelId: string;
terminalConnected?: boolean | null;
busOrBusbarSectionId: string;
connectablePosition: ConnectablePositionFormInfos;
q: number;
targetV: number;
targetDeadband: number;
sectionCount: number;
bPerSection: number;
qAtNominalV: number;
maximumSectionCount: number;
isLinear?: boolean | null;
properties: Record<string, string> | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the same static values that you used in the previous type; Like :

export const MAXIMUM_SECTION_COUNT = 'maximumSectionCount';

etc

import { Property } from '../common/properties/property-utils';
import { ConnectablePositionFormInfos } from '../../connectivity/connectivity.type';

export type ShuntCompensatorDialogSchemaBaseForm = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type ShuntCompensatorDialogSchemaBaseForm = {
export type ShuntCompensatorSchemaBaseForm = {

I don't think we need to have "Dialog" in the type name. A bit like LoadModificationSchemaForm in load-modification.type.ts

Same for the Form in ShuntCompensatorFormInfos. It is not included in LoadModificationInfos in load-modification.type.ts

Comment on lines +58 to +70
id: string;
name: string;
voltageLevelId: string;
terminalConnected?: boolean | null;
busOrBusbarSectionId: string;
connectablePosition: ConnectablePositionFormInfos;
q: number;
targetV: number;
targetDeadband: number;
sectionCount: number;
bPerSection: number;
qAtNominalV: number;
maximumSectionCount: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost all those data are AttributeModification in the database. It may be better to consider them AttributeModification in the front as well. Especially for the non mandatory attributes which may be set to UNSET in order to remove them.

For now I don't think that this would be used but this is done like this in LoadModificationInfos and I think this is more cautious.

Copy link
Contributor

@flomillot flomillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same remarks as #3619
(I will read the rest after the fixes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants