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.

Sign up for GitHub

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

Jump to bottom

[WIP] Add Monitoring - Alerts Overview and All Alerts listing [Depends on #12237] #11962

Closed
wants to merge 4 commits into from
Closed

[WIP] Add Monitoring - Alerts Overview and All Alerts listing [Depends on #12237] #11962

wants to merge 4 commits into from

Conversation

jeff-phillips-18
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 commented Oct 14, 2016 •
edited
Loading

This PR adds the Alerts Center section to the main navigation menu and sub-sections "Alerts" and "Group By Status". Please see Jira stories for this contribution for UX designs:

https://patternfly.atlassian.net/browse/CFUX-117
https://patternfly.atlassian.net/browse/CFUX-118
https://patternfly.atlassian.net/browse/CFUX-121
https://patternfly.atlassian.net/browse/CFUX-153
https://patternfly.atlassian.net/browse/CFUX-164

image

image

image

@serenamarie125 @alongoldboim @simon3z

@miq-bot add_label wip, enhancement

@simon3z
Copy link
Contributor

simon3z commented Oct 17, 2016

cc @zgalor @alongoldboim

@jeff-phillips-18 jeff-phillips-18 force-pushed the alerts branch 3 times, most recently from 8f72dfb to 109b629 Compare October 18, 2016 14:01
@miq-bot
Copy link
Member

miq-bot commented Oct 21, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Oct 21, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Oct 25, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@abonas
Copy link
Member

abonas commented Oct 27, 2016

Great to see this making alerts more accesible.
I am not sure that "sort by status" as a separate menu entry is intuitive, especially since there is an entry named alerts next to it (which can be sorted by status too) cc @theute

@serenamarie125
Copy link

@abonas, agreed. This is an initial design which is being iterated on.
More functionality will be moved from other areas to here, so rather than
worrying about terminology upfront, we decided to get the functionality in,
and rework terminology in a subsequent PR once we better understand the
bigger picture.

On Thursday, October 27, 2016, abonas [email protected] wrote:

Great to see this making alerts more accesible.
I am not sure that "sort by status" as a separate menu entry is intuitive,
especially since there is an entry named alerts next to it (which can be
sorted by status too) cc @theute https://github.com/theute

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11962 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AL7NU9AFt9t9bcL3CGQoQ36-FH0hzHV3ks5q4JsfgaJpZM4KXcaL
.

  • Serena Chechile Doyle
    UXD | Unified Management Experience
    Red Hat
    Cell 508-769-7715 | IRC - serena | Skype - serenamarie125 | Twitter -
    @serenamarie125

@miq-bot
Copy link
Member

miq-bot commented Nov 1, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@alongoldboim
Copy link
Contributor

@jeff-phillips-18 In alerts status when we're not getting a type of alert for a provider from the api server it will not be shown, I think we should always show all types including the icons and just place a 0.
selection_172

@serenamarie125
Copy link

@alongoldboim we did verify this with Thomas. They want to see all providers with issues at that point. We may be able to add something to the UI to toggle this behavior on or off, but there's no way to achieve that with our current filtering mechanism.

@alongoldboim
Copy link
Contributor

@serenamarie125 OK makes sense when i think about it :), so lets leave it as is for now.

@jeff-phillips-18 jeff-phillips-18 force-pushed the alerts branch 5 times, most recently from 3a5abc7 to d7720c1 Compare November 8, 2016 19:26
@miq-bot
Copy link
Member

miq-bot commented Nov 9, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@jeff-phillips-18 jeff-phillips-18 force-pushed the alerts branch 6 times, most recently from b2605bc to 6f4b14a Compare November 18, 2016 12:37
@jeff-phillips-18 jeff-phillips-18 changed the title [WIP] Add Alert Center - Alerts listing and Group by Status Add Monitoring - Alerts Overview and All Alerts listing [Depends on #12237] Nov 18, 2016
@jeff-phillips-18 jeff-phillips-18 mentioned this pull request Nov 18, 2016
Closed
@chessbyte chessbyte added the ui label Nov 21, 2016
@dclarizio
Copy link

@jeff-phillips-18 should this be WIP until we get some test coverage? I see those huge JS controllers with no tests. We also normally make the UI PRs WIP until the back end or API PRs are merged.
Thx, Dan

@jeff-phillips-18
Copy link
Member Author

@miq-bot add_label wip

@miq-bot miq-bot added the wip label Nov 21, 2016
@miq-bot
Copy link
Member

miq-bot commented Nov 21, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@chessbyte chessbyte changed the title Add Monitoring - Alerts Overview and All Alerts listing [Depends on #12237] [WIP] Add Monitoring - Alerts Overview and All Alerts listing [Depends on #12237] Nov 28, 2016
@jeff-phillips-18 jeff-phillips-18 force-pushed the alerts branch 5 times, most recently from 4424c74 to 4b2b1a0 Compare December 5, 2016 13:18
@jeff-phillips-18
Add Monitoring main navigation section, with Alert sections
05d932a 
This PR will provide viewing of alerts and the ability to
assign/unassign, acknowledge/unacknowledge, or add comments to alerts.
@jeff-phillips-18
Remove unnecessary dependency (ngResource)
f1069f4
@jeff-phillips-18
Add most recent alerts view
063a076 
Refactor common alerts functionality into new alertsCenterService
@moolitayer moolitayer mentioned this pull request Dec 6, 2016
Merged
@miq-bot
Copy link
Member

miq-bot commented Dec 7, 2016

Checked commits jeff-phillips-18/manageiq@05d932a~...aee249d with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
9 files checked, 1 offense detected

app/presenters/menu/default_menu.rb

@moolitayer moolitayer mentioned this pull request Dec 18, 2016
Merged
@miq-bot
Copy link
Member

miq-bot commented Dec 22, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

cben
cben reviewed Jan 11, 2017
View reviewed changes
app/services/alerts_service.rb

def alerts
result = {}
result.values
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this always return []?
[Sorry to barge in without reading most of this PR, ignore me if silly question]

@chessbyte
Copy link
Member

Please re-create PR in https://github.com/ManageIQ/manageiq-ui-classic

@chessbyte chessbyte closed this Jan 11, 2017
@moolitayer moolitayer mentioned this pull request Jan 16, 2017
Closed
@simon3z
Copy link
Contributor

simon3z commented Jan 25, 2017

@jeff-phillips-18 did you move this to https://github.com/ManageIQ/manageiq-ui-classic ? Can you drop here a reference? Thanks!

@jeff-phillips-18
Copy link
Member Author

@simon3z I did not move the PR since there is significant changes due to the updated API implementation. I am working on these changes and will put up a new PR in the classic-ui repo once I have it working again.

@jeff-phillips-18 jeff-phillips-18 mentioned this pull request Jan 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cben cben cben left review comments

Assignees

@dclarizio dclarizio

Labels
enhancement ui unmergeable wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jeff-phillips-18 @simon3z @miq-bot @abonas @serenamarie125 @alongoldboim @dclarizio @chessbyte @cben