Skip to content

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Aug 4, 2025

Depends on ManageIQ/manageiq-api#1293, which ensures that count actually is the full count we are looking for.

Fixes #9448

@kbrock Please review.

Depends on ManageIQ/manageiq-api#1293, which ensures that `count`
actually is the full count we are looking for.

Fixes ManageIQ#9448
@Fryguy Fryguy requested a review from a team as a code owner August 4, 2025 21:14
.then((data) => {
const notifications = data.resources.map(convert);

return {
notifications,
subcount: data.subcount,
subcount: data.count
Copy link
Member Author

@Fryguy Fryguy Aug 4, 2025

Choose a reason for hiding this comment

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

I was going to rename this key to count also, but that feels like a much more invasive change, so I left it for now.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Great change.

Fixing the count in the notifications query was definitely the right approach.

@jrafanie
Copy link
Member

jrafanie commented Aug 4, 2025

@asirvadAbrahamVarghese Can you look at the failures on this one? I think we were expecting the notifications calls in the failing tests and now they're gone due to the changes in this PR. Can you try this PR locally and see if it's the explicit wait on the notifications that's the problem:

cypress/e2e/ui/Settings/Application-Settings/edit_collect_logs.cy.js:    cy.wait('@getNotifications').then(() => {
     CypressError: Timed out retrying after 5000ms: `cy.wait()` timed out waiting `5000ms` for the 1st request to the route: `getNotifications`. No request ever occurred.

@Fryguy
Copy link
Member Author

Fryguy commented Aug 5, 2025

Oh I can remove them. I didn't realize we had waits on notifications

@asirvadAbrahamVarghese
Copy link
Contributor

asirvadAbrahamVarghese commented Aug 5, 2025

Can you look at the failures on this one? I think we were expecting the notifications calls in the failing tests and now they're gone due to the changes in this PR. Can you try this PR locally and see if it's the explicit wait on the notifications that's the problem:

Is this a blocker for an urgent release? PR #9516 will remove it

@Fryguy
Copy link
Member Author

Fryguy commented Aug 5, 2025

I can wait for #9516, but would like to get this is sooner than later. @jrafanie can you look at #9516?

@jrafanie
Copy link
Member

jrafanie commented Aug 5, 2025

Bumping CI after #9516 merge

@jrafanie jrafanie closed this Aug 5, 2025
@jrafanie jrafanie reopened this Aug 5, 2025
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

Nice, 💚 tests. Thanks @asirvadAbrahamVarghese. LGTM @Fryguy.

@jrafanie jrafanie merged commit b8b4140 into ManageIQ:master Aug 5, 2025
34 of 35 checks passed
@jrafanie jrafanie assigned jrafanie and unassigned kbrock Aug 5, 2025
@Fryguy Fryguy deleted the remove_extra_notifications_query branch August 5, 2025 19:42
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.

Only make one query to notifications
4 participants