Skip to content
This repository was archived by the owner on Sep 10, 2021. It is now read-only.

Conversation

mgrauer
Copy link
Contributor

@mgrauer mgrauer commented Apr 13, 2016

@cpatrick PTAL

This includes all the commits of #202. I can give you a demo tomorrow.

The behavior change is to split the aggregate metrics from notifications. Metrics defined by an aggregate metric spec will be calculated on all submissions. Notifications will only be set by an aggregate metric notification, which defines a threshold and a branch, and can have users linked to it who will be notified when a metric is over threshold.

cpatrick and others added 30 commits March 17, 2016 11:41
* Remove getDistinctBranchesForMetricName from TrendModel.
* Fix bad ordering query in SubmissionModel.
* origin/master:
  Add branch and metric textarea to user alerts UI
  Add branch name to aggregate metric notification email
  Add warning for definition edit of Tracker AMS
@mgrauer mgrauer changed the title Aggregregate metrics on all branches [WIP] Aggregregate metrics on all branches Apr 13, 2016
@mgrauer
Copy link
Contributor Author

mgrauer commented Apr 13, 2016

One more TODO, test that notification jobs are correctly scheduled when aggregate metrics are computed on a submission via the ApiComponent.

@mgrauer mgrauer changed the title [WIP] Aggregregate metrics on all branches Aggregregate metrics on all branches Apr 13, 2016
/** @var Tracker_AggregateMetricNotificationDao $aggregateMetricNotificationDao */
$aggregateMetricNotificationDao = $aggregateMetricNotificationModel->load($aggregateMetricNotificationId);
$userDao = $apihelperComponent->getUser($args);
if ($aggregateMetricSpecModel->policyCheck($aggregateMetricNotificationDao->getAggregateMetricSpec(), $userDao, MIDAS_POLICY_WRITE) === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

MIDAS_POLICY_READ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's where the READ policy change should be

@mgrauer
Copy link
Contributor Author

mgrauer commented Apr 13, 2016

@cpatrick

GET now requires READ

@mgrauer
Copy link
Contributor Author

mgrauer commented Apr 14, 2016

This should be ok to merge from a conflict perspective.

@mgrauer mgrauer merged commit 1a86288 into master Apr 15, 2016
@mgrauer
Copy link
Contributor Author

mgrauer commented Apr 15, 2016

Closing this PR as all new commits were merged in #231.

@mgrauer mgrauer deleted the aggregregate_metrics_on_all_branches branch April 15, 2016 22:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants