[3389] Add support for DateTime widget #3390

lfasani · 2024-04-16T10:02:55Z

Bug: #3389

Pull request template

General purpose

What is the main goal of this pull request?

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).

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?

AxelRICHARD

first review of the code, I didn't test it yet.

doc/iterations/2024.5/shapes/add_date_time_widget.adoc

...a/org/eclipse/sirius/components/formdescriptioneditors/components/DateTimeStyleProvider.java

...main/java/org/eclipse/sirius/components/view/form/provider/GroupDescriptionItemProvider.java

...form-edit/src/main/java/org/eclipse/sirius/components/view/form/provider/FormEditPlugin.java

...s-forms/src/main/java/org/eclipse/sirius/components/forms/elements/DateTimeElementProps.java

packages/sirius-web/backend/sirius-web/src/main/resources/application.properties

packages/view/backend/sirius-components-view-form/src/main/resources/model/form.ecore

AxelRICHARD · 2024-04-16T12:19:10Z

Could you please rebase this PR on master instead on one of your own branch?

sbegaudeau · 2024-04-16T12:58:06Z

...orms/frontend/sirius-components-forms/src/propertysections/DateTimeWidgetPropertySection.tsx

+function useErrorReporting<T>(
+  result: MutationResult<T>,
+  extractPayload: (data: T | null) => GQLPayload | undefined,
+  addErrorMessage: (message: string) => any,
+  addMessages: (messages: any) => any
+) {
+  useEffect(() => {
+    const { loading, data, error } = result;
+    if (!loading) {
+      if (error) {
+        addErrorMessage(error.message);
+      }
+      const payload: GQLPayload | undefined = extractPayload(data || null);
+      if (payload && (isSuccessPayload(payload) || isErrorPayload(payload))) {
+        const { messages } = payload;
+        addMessages(messages);
+      }
+    }
+  }, [result.loading, result.data, result.error]);
+}


Something like this should probably be a hook in sirius-components-core directly to be usable by all downstream projects. The hook in question should have the dependency to addErrorMessage / addMessages and given the pattern used by our mutation data fetchers (on purpose), I'm pretty sure you don't need anyone to tell you how to extract the payload.

yes but not in the PR. We must define the scope of this change because currently useErrorReporting pattern is used in portal, deck, gantt and DateTimeWidget. Ideally, we should use it everywhere it is needed. It implies lots of changes including GQLPayloadXxx that are define everywhere.

We could simplify useErrorReporting by using useMultiToast inside. Then we would have only result and extractPayload parameters.
We could even force all GQLxxxMutationData to have a payload attribute of type GQLPayload (defined in sirius-components-core) and then only the result parameter would be necessary. (isErrorPayload and isSuccessPayload would be included in useErrorReporting too

yes but not in the PR. We must define the scope of this change because currently useErrorReporting pattern is used in portal, deck, gantt and DateTimeWidget.

Yes in this PR. I don't know why this kind of situation happens again and again where people try to improve just their own piece of code. This code should either be removed or in sirius-components-core and that's the same for its other occurences.

It implies lots of changes including GQLPayloadXxx that are define everywhere.

I'm not asking for all pieces of code where errors are treated to be updated, I'm just asking for this specific piece of code, those 20 lines of code, to be accessible by everybody by being in sirius-components-core. And if they are moved there, it should be very easy to update both the portal and gantt part too instead of copying and pasting stuff.

sbegaudeau

I'll handle the comments that I've made and update the PR accordingly

sbegaudeau · 2024-05-03T14:08:22Z

packages/core/frontend/sirius-components-core/src/graphql/GQLTypes.types.ts

@@ -16,6 +16,7 @@ export interface GQLMessage {
  level: string;
 }

-export interface GQLErrorPayload {
+export interface GQLPayload {


There's no generic GQLPayload in the GraphQL API with such signature

I've removed it since it was wrong

packages/core/frontend/sirius-components-core/src/toast/useReporting.ts

packages/deck/frontend/sirius-components-deck/src/representation/useDeckMutation.types.ts

packages/deck/frontend/sirius-components-deck/src/representation/useDeckMutations.ts

...mdescriptioneditors/frontend/sirius-components-formdescriptioneditors/src/DateTimeWidget.tsx

packages/gantt/frontend/sirius-components-gantt/src/graphql/mutation/useGanttMutations.ts

packages/portals/frontend/sirius-components-portals/src/hooks/usePortalMutations.ts

packages/portals/frontend/sirius-components-portals/src/hooks/usePortalMutations.types.ts

...iew-emf/src/main/java/org/eclipse/sirius/components/view/emf/form/DateTimeStyleProvider.java

packages/portals/frontend/sirius-components-portals/src/hooks/usePortalMutations.types.ts

sbegaudeau · 2024-05-06T08:00:22Z

packages/deck/frontend/sirius-components-deck/src/representation/useDeckMutations.ts

@@ -259,7 +237,7 @@ export const useDeckMutations = (editingContextId: string, representationId: str
  const [rawcreateCard, rawCreateCardResult] = useMutation<GQLCreateCardData, GQLCreateCardVariables>(
    createCardMutation
  );
-  useErrorReporting(rawCreateCardResult, (data) => data?.createCard);
+  useReporting(rawCreateCardResult, (data: GQLCreateCardData) => data.createCard);


This is wrong, it's not your fault, the error has been on master for quite some time. We have a simple pattern where everything follows a logic yet people want to change things and they make mistakes. The name of the mutation is not createCard so the name of the payload, type etc should change. Because of that, here we are extracting a payload which do not exist. The existing code either did not properly execute the code or made incorrect assumptions.

Bug: #3389 Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

lfasani added pr: to review later priority: low labels Apr 16, 2024

lfasani added this to the 2024.5.0 milestone Apr 16, 2024

lfasani self-assigned this Apr 16, 2024

lfasani requested review from pcdavid, sbegaudeau and AxelRICHARD as code owners April 16, 2024 10:02

lfasani linked an issue Apr 16, 2024 that may be closed by this pull request

Add support for DateTime widget #3389

Closed

1 task

AxelRICHARD reviewed Apr 16, 2024

View reviewed changes

sbegaudeau requested changes Apr 16, 2024

View reviewed changes

lfasani force-pushed the lfa/enh/dateTimeWidget branch from be56d56 to 29db510 Compare April 16, 2024 13:31

lfasani requested a review from gcoutable as a code owner April 16, 2024 13:31

lfasani changed the base branch from lfa/enh/expand to master April 16, 2024 13:31

lfasani force-pushed the lfa/enh/dateTimeWidget branch 3 times, most recently from e29fc09 to a3691f8 Compare April 17, 2024 10:45

lfasani requested a review from frouene April 17, 2024 12:13

lfasani added pr: to review soon and removed pr: to review later labels Apr 17, 2024

lfasani requested a review from AxelRICHARD April 17, 2024 12:29

lfasani force-pushed the lfa/enh/dateTimeWidget branch from 0f6187f to 3074ded Compare April 23, 2024 09:18

lfasani requested a review from sbegaudeau April 23, 2024 09:26

lfasani force-pushed the lfa/enh/dateTimeWidget branch 3 times, most recently from 36ba2bc to bb61155 Compare April 23, 2024 13:20

sbegaudeau changed the title ~~Add support for DateTime widget~~ [3389] Add support for DateTime widget Apr 25, 2024

sbegaudeau force-pushed the lfa/enh/dateTimeWidget branch from bb61155 to 5f60759 Compare April 29, 2024 14:28

lfasani force-pushed the lfa/enh/dateTimeWidget branch from 5f60759 to 5fcaf96 Compare April 30, 2024 12:52

lfasani linked an issue Apr 30, 2024 that may be closed by this pull request

Use DateTime widget for task properties #3441

Closed

1 task

sbegaudeau requested changes May 3, 2024

View reviewed changes

sbegaudeau reviewed May 5, 2024

View reviewed changes

packages/portals/frontend/sirius-components-portals/src/hooks/usePortalMutations.types.ts Outdated Show resolved Hide resolved

sbegaudeau force-pushed the lfa/enh/dateTimeWidget branch 2 times, most recently from 130a461 to 179b13d Compare May 5, 2024 19:39

sbegaudeau reviewed May 6, 2024

View reviewed changes

sbegaudeau force-pushed the lfa/enh/dateTimeWidget branch from 179b13d to a74404d Compare May 6, 2024 08:08

lfasani and others added 2 commits May 6, 2024 10:45

[3389] Add support for DateTime widget

e94e244

Bug: #3389 Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>

[test] Restore the previous version of the Papaya domain

e3d59a3

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

sbegaudeau force-pushed the lfa/enh/dateTimeWidget branch from a74404d to e3d59a3 Compare May 6, 2024 08:46

sbegaudeau approved these changes May 6, 2024

View reviewed changes

sbegaudeau merged commit 3717bb4 into master May 6, 2024
4 checks passed

sbegaudeau deleted the lfa/enh/dateTimeWidget branch May 6, 2024 08:46

lfasani mentioned this pull request May 13, 2024

Use DateTime widget for task properties #3441

Closed

1 task

This was unlinked from issues May 13, 2024

Add support for DateTime widget #3389

Closed

Use DateTime widget for task properties #3441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3389] Add support for DateTime widget #3390

[3389] Add support for DateTime widget #3390

lfasani commented Apr 16, 2024

AxelRICHARD left a comment

AxelRICHARD commented Apr 16, 2024 •

edited

sbegaudeau Apr 16, 2024

lfasani Apr 17, 2024

lfasani Apr 17, 2024 •

edited

sbegaudeau Apr 22, 2024

lfasani Apr 23, 2024

sbegaudeau left a comment

sbegaudeau May 3, 2024

sbegaudeau May 4, 2024

sbegaudeau May 6, 2024

[3389] Add support for DateTime widget #3390

[3389] Add support for DateTime widget #3390

Conversation

lfasani commented Apr 16, 2024

Pull request template

General purpose

Project management

Architectural decision records (ADR)

Dependencies

Frontend

General purpose

Typing

Backend

General purpose

Architecture

Review

How to test this PR?

AxelRICHARD left a comment

Choose a reason for hiding this comment

AxelRICHARD commented Apr 16, 2024 • edited

sbegaudeau Apr 16, 2024

Choose a reason for hiding this comment

lfasani Apr 17, 2024

Choose a reason for hiding this comment

lfasani Apr 17, 2024 • edited

Choose a reason for hiding this comment

sbegaudeau Apr 22, 2024

Choose a reason for hiding this comment

lfasani Apr 23, 2024

Choose a reason for hiding this comment

sbegaudeau left a comment

Choose a reason for hiding this comment

sbegaudeau May 3, 2024

Choose a reason for hiding this comment

sbegaudeau May 4, 2024

Choose a reason for hiding this comment

sbegaudeau May 6, 2024

Choose a reason for hiding this comment

AxelRICHARD commented Apr 16, 2024 •

edited

lfasani Apr 17, 2024 •

edited