Skip to content

Comments

Enrich process config creation#41

Open
FranckLecuyer wants to merge 1 commit intomainfrom
enrich_process_config
Open

Enrich process config creation#41
FranckLecuyer wants to merge 1 commit intomainfrom
enrich_process_config

Conversation

@FranckLecuyer
Copy link
Contributor

@FranckLecuyer FranckLecuyer commented Feb 13, 2026

PR Summary

Add owner, creation date, last modification date and last user that did a modification

coderabbitai[bot]

This comment was marked as outdated.

@FranckLecuyer FranckLecuyer changed the title Enrich process config creation [WIP] Enrich process config creation Feb 13, 2026
coderabbitai[bot]

This comment was marked as outdated.

@gridsuite gridsuite deleted a comment from coderabbitai bot Feb 16, 2026
@FranckLecuyer FranckLecuyer changed the title [WIP] Enrich process config creation Enrich process config creation Feb 16, 2026
@coderabbitai

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

…n user to the process configs

Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
@sonarqubecloud
Copy link

Copy link
Contributor

@antoinebhs antoinebhs left a comment

Choose a reason for hiding this comment

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

This PR asks the question where we want to save this data.
In the Gridsuite ecosystem, we do not save this data in all elements available in Gridexplore, it's handled by the directory server.
We need to discuss with the PO if we want in monitor to have our "ProcessConfig" explorer that do not use Gridexplore or if we want to use Gridexplore and instead of implementing it like that, we want a new GRidexplore object.
I will finish the review depending on what we choose.

/**
* @author Franck Lecuyer <franck.lecuyer at rte-france.com>
*/
public final class Constants {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic for now is to put in monitor-commons class that are commons to server and worker.
I see that these constants are only used in the server. So I would put this file in server/Constants.java

*/
@Getter
public abstract class AbstractProcessConfig implements ProcessConfig {
@JsonProperty(required = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I don't really understand the logic here: why do we make it required but also add a null check:
        this.modificationUuids = modificationUuids != null ? List.copyOf(modificationUuids) : null;
  • Why do we need an abstraction and can't continue using the record?

List<UUID> modificationUuids();
List<UUID> getModificationUuids();

@JsonProperty(access = JsonProperty.Access.READ_ONLY)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is readonly to avoid API requests with this data, right?

) implements ProcessConfig {
@Getter
@EqualsAndHashCode(callSuper = true)
public class SecurityAnalysisConfig extends AbstractProcessConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment for the record vs class and null/required

@OrderColumn(name = "pos_modifications")
private List<UUID> modificationUuids;

@Column(name = "owner", length = 80, nullable = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

we usually don't constrain in the bd but it see that it's consistent with directory server

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.

2 participants