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

[IMPAC-578] Create Widget KPIs #306

Merged
merged 2 commits into from
Jun 1, 2017
Merged

[IMPAC-578] Create Widget KPIs #306

merged 2 commits into from
Jun 1, 2017

Conversation

xaun
Copy link
Contributor

@xaun xaun commented May 31, 2017

@cesar-tonnoir please review. Marked WIP as the #update & #delete actions are not validated as still working / updated.

Copy link
Contributor

@cesar-tonnoir cesar-tonnoir left a comment

Choose a reason for hiding this comment

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

LGTM - @ouranos fyi

Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

LGTM
@xaun Is it backward compatible:

  • with a MnoHub without maestrano/maestrano-hub#514 ?
  • with impac-angular?

Could you rename the PR to include the JIRA number as well as mention in the description the PRs on other projects?

Thanks

@ouranos
Copy link
Contributor

ouranos commented Jun 1, 2017

Also, are we missing specs here?

@xaun xaun changed the title WIP: Update kpis_controller for widget KPIs [IMPAC-578] Create Widget KPIs Jun 1, 2017
@xaun
Copy link
Contributor Author

xaun commented Jun 1, 2017

Linked PRs:
maestrano/maestrano-hub#514
maestrano/impac-angular#338

@xaun
Copy link
Contributor Author

xaun commented Jun 1, 2017

@ouranos can you please re-review. The "backward compatiblity fix" for mnoe is a bit strange, when mno-hub is without the changes is maestrano-hub#515, mnoe kpi template errors with ActionView::Template::Error (undefined method 'map' for #<MnoEnterprise::Impac::Alert(alerts) >. I would expected the her :has_many relationship to manage this? Let me know if there is a better way to handle / guard this.
Backwards compatibility with impac-angular confirmed.

Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

Ah I had the same issue in #305

Can you check if any? work better?
Haven't found a better way either :/

it "creates kpi alerts" do
subject
expect(assigns(:kpi).alerts).to eq([alert])
expect(response.code).to eq('200')
Copy link
Contributor

Choose a reason for hiding this comment

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

expect(response).to have_http_status(:ok)

end
end

it { subject; expect(response.code).to eq('200') }
Copy link
Contributor

Choose a reason for hiding this comment

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

it { is_expected.to have_http_status(:ok) }

@cesar-tonnoir cesar-tonnoir merged commit a3c78dc into maestrano:master Jun 1, 2017
@xaun xaun deleted the feature/widget-kpis branch June 7, 2017 08:58
aluqueGH pushed a commit to aluqueGH/mno-enterprise that referenced this pull request Jul 10, 2018
…anding-pages

Public Pages - fix redirect logic
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