Skip to content

[3679] Split representation metadata from content #3864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

gcoutable
Copy link
Member

@gcoutable gcoutable commented Aug 13, 2024

Pull request template

General purpose

What is the main goal of this pull request?

  • Bug fixes
  • New features
  • Documentation
  • Cleanup
  • Tests
  • Build / releng

Project management

  • Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
  • Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
  • Have the relevant issues been added to the pull request?
  • Have the relevant labels been added to the issues? (area:, difficulty:, type:)
  • Have the relevant issues been added to the same project and milestone as the pull request?
  • Has the CHANGELOG.adoc been updated to reference the relevant issues?
  • Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
  • In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

  • Does the title of the commit contributing the ADR start with [doc]?
  • Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

  • Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
  • Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

  • Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

  • Variables have a proper type
  • Functions’ arguments have a proper type
  • Functions’ return type are specified
  • Hooks are properly typed:
    • useMutation<DATA_TYPE, VARIABLE_TYPE>(…)
    • useQuery<DATA_TYPE, VARIABLE_TYPE>(…)
    • useSubscription<DATA_TYPE, VARIABLE_TYPE>(…)
    • useMachine<CONTEXT_TYPE, EVENTS_TYPE>(…)
    • useState<STATE_TYPE>(…)
  • All components have a proper typing for their props
  • No useless optional chaining with ?. (if the GraphQL API specifies that a field cannot be null, do not treat it has potentially null for example)
  • Nullable values have a proper type (for example let diagram: Diagram | null = null;)

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

  • Are all the event handlers tested?
  • Are the event processor tested?
  • Is the business code (services) tested?
  • Are diagram layout changes tested?

Architecture

  • Are data structure classes properly separated from behavioral classes?
  • Are all the relevant fields final?
  • Is any data structure mutable? If so, please write a comment indicating why
  • Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Please describe here the various use cases to test this pull request

  • Has the Kiwi TCMS test suite been updated with tests for this contribution?

@gcoutable gcoutable added this to the 2024.9.0 milestone Aug 13, 2024
@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch 4 times, most recently from a39dc9d to 4c813fd Compare August 14, 2024 14:14
@gcoutable gcoutable modified the milestones: 2024.9.0, 2024.11.0 Sep 16, 2024
@sbegaudeau sbegaudeau force-pushed the gco/enh/replace-use-of-representation-find-by branch from 90748e6 to 5e487c3 Compare September 23, 2024 15:07
Base automatically changed from gco/enh/replace-use-of-representation-find-by to master September 23, 2024 15:22
@sbegaudeau
Copy link
Member

@gcoutable Could you rebase that PR once you are done with a first version of the command palette? Thanks

@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch 10 times, most recently from c2529cd to 3b145dc Compare September 27, 2024 08:00
@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch 4 times, most recently from a178a5f to 4756e92 Compare October 10, 2024 13:40
@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch from 4756e92 to 8c2c366 Compare October 14, 2024 08:56
@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch from 8c2c366 to 8a8984a Compare October 22, 2024 09:03
Copy link
Member

@sbegaudeau sbegaudeau left a comment

Choose a reason for hiding this comment

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

I've not tested the code but if all the tests are green, we can be confident that it's not breaking everything :)

@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch 5 times, most recently from 9e4db16 to 57bf1a2 Compare October 23, 2024 15:37
@gcoutable gcoutable requested a review from sbegaudeau October 23, 2024 16:06
@gcoutable
Copy link
Member Author

Regarding the fact that we are creating representation metadata and representation content in the RepresentationPersistenceService, I put the PersistenceService split in the next PR since the time I did that second PR I was stuck on that part.
If you want, I can retrieve from the next PR, the PersistenceService split.

@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch 2 times, most recently from 798c9f5 to 70b74f3 Compare October 24, 2024 08:20
@gcoutable gcoutable force-pushed the gco/enh/split-representation-data branch 2 times, most recently from abf1b9c to 0a9058e Compare October 24, 2024 15:49
@sbegaudeau
Copy link
Member

Regarding the fact that we are creating representation metadata and representation content in the RepresentationPersistenceService, I put the PersistenceService split in the next PR since the time I did that second PR I was stuck on that part. If you want, I can retrieve from the next PR, the PersistenceService split.

Ok I can live with this if it comes in the next PR but updating two bounded contexts in the same transaction should never be done. If the two bounded contexts are tightly connected, they may be one bounded contexts with multiple aggregate. In this case, I wouldn't be surprised to see both RepresentationMetadata and RepresentationContent consolidated in a single bounded context representations. We would just move the code, we wouldn't change anything else.

We will see after the other PR(s).

Bug: #3679
Signed-off-by: Guillaume Coutable <guillaume.coutable@obeo.fr>
@sbegaudeau sbegaudeau force-pushed the gco/enh/split-representation-data branch from 0a9058e to f2b010e Compare October 25, 2024 09:13
@gcoutable
Copy link
Member Author

Ok I can live with this if it comes in the next PR but updating two bounded contexts in the same transaction should never be done. If the two bounded contexts are tightly connected, they may be one bounded contexts with multiple aggregate. In this case, I wouldn't be surprised to see both RepresentationMetadata and RepresentationContent consolidated in a single bounded context representations. We would just move the code, we wouldn't change anything else.

We will see after the other PR(s).

I tried the change yesterday, and since that was working I pushed it in that PR.
Now every create event handler are doing a save for metadata then a save for the content.

@sbegaudeau sbegaudeau merged commit b4bfc23 into master Oct 25, 2024
4 checks passed
@sbegaudeau sbegaudeau deleted the gco/enh/split-representation-data branch October 25, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants