Skip to content
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

feat: [DHIS2-18018][DHIS2-17853] show related stages widget in view event page #3916

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

simonadomnisoru
Copy link
Contributor

@simonadomnisoru simonadomnisoru commented Dec 19, 2024

DHIS2-18018

Tech summary:

  • Make the WidgetRelatedStages configurable in the enrollmentEventEditLayout page layout
  • Based on the user selected option show an action button in WidgetRelatedStages
  • use react-query to create the linked event and the relationship in useAddEventWithRelationship

Copy link

github-actions bot commented Jan 8, 2025

@simonadomnisoru simonadomnisoru marked this pull request as ready for review January 8, 2025 09:10
@simonadomnisoru simonadomnisoru requested a review from a team as a code owner January 8, 2025 09:10
Copy link
Contributor

@henrikmv henrikmv left a comment

Choose a reason for hiding this comment

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

Hi Simona,
I’ve reviewed this feature and identified a few findings that I hope you can take a look at. I wasn’t able to locate any design documents or overarching plans for this feature, so please let me know if I’ve misunderstood anything. Thanks!


return (
<div className={classes.link}>
<Button secondary small onClick={onLink} disabled={disabled}>
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: There may be a design document on this, but this is perhaps a button that should typically be primary? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no design document for this feature. I have implemented your suggestion. Thanks for the feedback!

Comment on lines 182 to 183
const disabled = saveAttempted && Object.values(errorMessages).filter(Boolean).length !== 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

The LinkButton stays enabled until clicked, even if required fields are empty. Can we remove the saveAttempted variable so it’s always disabled when required fields are incomplete?

Suggested change
const disabled = saveAttempted && Object.values(errorMessages).filter(Boolean).length !== 0;
const disabled = Object.values(errorMessages).some(Boolean);

onUpdateEnrollmentSuccess({});

if (payload.linkMode === relatedStageActions.ENTER_DATA && payload.eventIdToRedirectTo) {
onNavigateToEvent(payload.eventIdToRedirectTo);
Copy link
Contributor

Choose a reason for hiding this comment

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

When creating a relationship via Schedule or Link to existing event, there’s no confirmation or redirection. Is that the intended behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the intended behavior. When creating a relationship via Schedule or Link to existing event, the linked event will appear with a blue background above the currently open event.

Screen.Recording.2025-01-23.at.15.39.07.mov


export const WidgetRelatedStagesWorkspace: WidgetConfig = {
Component: WidgetRelatedStages,
shouldHideWidget: ({ currentPage }) => currentPage === EnrollmentPageKeys.EDIT_EVENT,
Copy link
Contributor

Choose a reason for hiding this comment

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

The widget is being displayed even though a relationship already exists. Attempting to create a new relationship results in a 409 error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a fix for the 409 error, but I couldn't reproduce the case where the widget is being displayed even though a relationship already exists. Could you please provide the steps? Thank you!

Base automatically changed from DHIS2-17192 to master January 24, 2025 13:51
@simonadomnisoru simonadomnisoru changed the title feat: [DHIS2-18018] show related stages widget in view event page feat: [DHIS2-18018][DHIS2-17853] show related stages widget in view event page Jan 27, 2025
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 New issues
2 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

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

Copy link
Contributor

@eirikhaugstulen eirikhaugstulen left a comment

Choose a reason for hiding this comment

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

Hey @simonadomnisoru!
I'm getting some strange behavior if the relationship is not bidirectional. It seems to not update/navigate correctly in my app. Could you take a look and see if you get the same conclusion in your env? Otherwise, this looks great!

@@ -26,6 +28,10 @@ export const DefaultPageLayout: PageLayoutConfig = {
type: WidgetTypes.COMPONENT,
name: 'EditEventWorkspace',
},
{
type: WidgetTypes.COMPONENT,
name: 'WidgetRelatedStagesWorkspace',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this name to just RelatedStagesWorkspace? None of the other names contains widget, so just trying to stay consistent. (Sorry, this was probably done ages ago)

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

Successfully merging this pull request may close these issues.

3 participants