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

fix: [DHIS2-16010] app crashes on invalid programid #3765

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

Conversation

alaa-yahia
Copy link
Member

@alaa-yahia alaa-yahia commented Aug 19, 2024

Implements DHIS2-16010

  • Ensure the context selector is consistently available on all pages when programId || orgUnitId is invalid.
  • Update withErrorMessageHandler to use DHIS2 UI NoticeBox of type error

Enrollment Page

  • Keep context selector always present
  • Display NoticeBox if there is invalid programmed || enrollementId || teiId

EnrollmentEventNew Page

  • Redirect to enrollment page when there is invalid programId

Main Page:

  • Display Notice box if there is invalid orgUnitId || programId

New Page

  • Ensure context selector is always present when there isn’t a valid programId || orgUnitId
  • Display Notice box if there is invalid orgUnitId || programId

@alaa-yahia alaa-yahia requested a review from a team as a code owner August 19, 2024 15:27
Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

Hi @alaa-yahia,
Along with this fix, let's enable the related disabled cypress scenario. Additionally, is it possible to add another scenario for the enrollment page too?
Thank you!

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 @alaa-yahia!
If you try to change to an invalid enrollmentId or teiId with this change, it seems like it's stuck in a loading state? Could you have a look at this? 😊 I also struggle a bit when using enrollmentId=AUTO after this change, but it seems a bit more sporadic.

Before:
image

After:
image

Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

@alaa-yahia
Copy link
Member Author

Hey @alaa-yahia! If you try to change to an invalid enrollmentId or teiId with this change, it seems like it's stuck in a loading state? Could you have a look at this? 😊 I also struggle a bit when using enrollmentId=AUTO after this change, but it seems a bit more sporadic.

Before: image

After: image

Hey @eirikhaugstulen I have updated the code.

@simonadomnisoru
Copy link
Contributor

Hey @alaa-yahia! If you try to change to an invalid enrollmentId or teiId with this change, it seems like it's stuck in a loading state? Could you have a look at this? 😊 I also struggle a bit when using enrollmentId=AUTO after this change, but it seems a bit more sporadic.

Hi @eirikhaugstulen,
Can you please re-check if the latest commits fixed these issues you reported? For me, everything seems fine now.
Thank you! 🙏

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 @alaa-yahia - so sorry for taking such a long time before getting back to you. It must have gone over my head. Feel free to ping me directly if you're blocked in the future. This looks good now 🎉

@alaa-yahia
Copy link
Member Author

Hey @alaa-yahia - so sorry for taking such a long time before getting back to you. It must have gone over my head. Feel free to ping me directly if you're blocked in the future. This looks good now 🎉

I forgot all about it either @eirikhaugstulen, otherwise I would have pinged you! No worries at all about the delay. Thank you for the review.

Copy link

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

Successfully merging this pull request may close these issues.

3 participants