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) O3-4183: Improve visit queue number attribute UUID handling in queue table #1366

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

jwnasambu
Copy link
Contributor

@jwnasambu jwnasambu commented Nov 12, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

I modified the code to log an error to the console when visitQueueNumberAttributeUuid is not configured and omit the column from the queue table.

Screenshots

Related Issue

https://openmrs.atlassian.net/browse/O3-4183

Other

@jwnasambu
Copy link
Contributor Author

@ibacher kindly feel free to review my PR at your convenient time, please! Through the help of the coffee break team, I was unable to reproduce the bug. Kindly what could be the steps to reproduce the issue?

@denniskigen denniskigen requested a review from ibacher November 12, 2024 20:12
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

LTGM. Can you resolve the conflicts please?

@NethmiRodrigo NethmiRodrigo changed the title Change the visitQueueNumberAttributeUuid not configured string in the service queues app (fix) O3-4183: Change the visitQueueNumberAttributeUuid not configured string in the service queues app Jan 16, 2025
Comment on lines 42 to 44
className={`${styles.admissionRequestsContainer} ${
inpatientRequests?.length ? styles.blackBackground : styles.lightBlueBackground
}`}>
Copy link
Member

Choose a reason for hiding this comment

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

nit: Prefer using classnames here.

Copy link
Contributor Author

@jwnasambu jwnasambu Jan 19, 2025

Choose a reason for hiding this comment

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

@denniskigen Kindly, I am somehow confused. This is not part of my changes, though I don't mind fixing the issue. I made changes on one file and would like to know why I have 8 files changed.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like changes from linting

Copy link
Member

Choose a reason for hiding this comment

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

Resolved in this commit. For whatever reason, some files in the Ward app were not getting linted using our prettier config. I've fixed that. So all you need to do is rebase against main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denniskigen Thanks for the fix and follow-up. I have fixed the merge conflicts, too.

@denniskigen
Copy link
Member

denniskigen commented Jan 22, 2025

LGTM. Thanks. Maybe a PR title like (fix) O3-4183: Improve visit queue number attribute UUID handling in queue table explains the changes in this diff better?

@jwnasambu jwnasambu changed the title (fix) O3-4183: Change the visitQueueNumberAttributeUuid not configured string in the service queues app (fix) O3-4183: Improve visit queue number attribute UUID handling in queue table Jan 22, 2025
@jwnasambu
Copy link
Contributor Author

@denniskigen I consent your suggestion is much better compared to what I had before.

@denniskigen denniskigen merged commit 1f111fc into openmrs:main Jan 23, 2025
6 checks passed
@denniskigen
Copy link
Member

denniskigen commented Jan 23, 2025

Thanks, @jwnasambu!

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.

3 participants