[IMPAC-335] Multiple Recipients - #292
Conversation
aabb2e2 to
3161de3
Compare
adamaziz15
changed the title
|
@xaun @cesar-tonnoir I have begun modifying the frontend to send up multiple recipients when creating an alert. My first concern was to make sure the data was going to mno-hub properly so that it could be used to create the alert. This is where I got a little stuck, I think her is removing the data from params. I am not too familiar with it so I am still figuring things out. 😅 Update: I think I'm on the right track. Let me know if you guys think I should approach this differently. It looks like I'd have to change the alerts controller a bit in mnoe to allow for the recipients to be passed through to the resources controller in mno-hub. |
6d9a3a3 to
1056ec8
Compare
|
|
||
| for alert in alertsToCreate | ||
| promises.push ImpacAlerts.create(kpi.id, { alert: _.pick(alert, ['service']) }) | ||
| promises.push ImpacAlerts.create(kpi.id, { alert: _.pick(alert, ['service']), recipients: alert.recipients }) |
There was a problem hiding this comment.
@adamaziz15 an in-app alert wont have key recipients, maybe do a merge if service is email instead.
|
@xaun I've improved the UI to initialize org members as active or inactive so that in the alerts modal it will show who is already receiving the alert. I've also added the appropriate functions to all of the services needed to allow for updating alerts. |
adamaziz15
changed the title
|
@cesar-tonnoir @xaun Should be ready to go 👍 , Please Review. |
xaun
left a comment
xaun
left a comment
There was a problem hiding this comment.
Hi Adam, good work with this 👍 here are some suggested changes :)
| $compile(alertsConfig.contents())(scope) | ||
|
|
||
| controller: ($scope) -> | ||
| $scope.members = [{email: "example@maestrano.io"},{email: "example@maestrano.io"},{email: "example@maestrano.io"},{email: "example@maestrano.io"}] |
There was a problem hiding this comment.
Should probably remove these hardcoded members :)
| (config) -> | ||
| $scope.members = config.currentOrgMembers | ||
| #Sets current state of recipients for email alerts | ||
| email_alert = _.filter($scope.kpi.alerts, (alert) -> alert.service == 'email')[0] |
There was a problem hiding this comment.
_.find() would be better here for what you're doing as it returns the first match.
There was a problem hiding this comment.
Also, this is javascript/coffeescript, no snake-case unless accessing an object property returned from a ruby API. Please rename to emailAlert.
| for member in $scope.members | ||
| member.active = true if email_recipients.includes(member.id) | ||
| else | ||
| _.filter($scope.members, (member) -> member.email == config.userData.email)[0].active = true |
There was a problem hiding this comment.
If for some reason $scope.members doesn't contain the currentUsers email how will this behave?
Should do something like?:
currentUserMember = _.find($scope.members, (member) -> member.email == config.userData.email)
currentUserMember.active = true if currentUserMember
| label: "By sending me an email to:" | ||
| } | ||
|
|
||
| $scope.search = { text: "" } |
There was a problem hiding this comment.
Maybe we should give this a more specific name?
| #Sets current state of recipients for email alerts | ||
| email_alert = _.filter($scope.kpi.alerts, (alert) -> alert.service == 'email')[0] | ||
| if email_alert | ||
| email_recipients = email_alert.recipients.map((recipient) -> recipient.id) |
There was a problem hiding this comment.
can we rename this emailAlertRecipients
|
|
||
| $scope.onKeyPress = (event) -> | ||
| if event.which == 13 | ||
| recipient = _.filter($scope.members, (member) -> member.email == $scope.search.text)[0] |
| {{member.email}} | ||
| <button type="button" class="close" aria-label="Close"> | ||
| <span aria-hidden="true">×</span> | ||
| </button> |
There was a problem hiding this comment.
Doesn't look like this remove button is hooked up to a method?
There was a problem hiding this comment.
Because the ng-click directive is on the < li > element it is triggered when the button is clicked. The button is really there more for UX purposes as it makes clearer which members are receiving the alert as well as how to remove them.
|
|
||
| <ul class="list-group recipients-list available" ng-show="showRecipientList(alert)"> | ||
|
|
||
| <li class="list-group-item" ng-repeat="member in members | filter:search.text" ng-hide="member.active" ng-click="toggleRecipient(member)"> |
There was a problem hiding this comment.
Like above, ng-if="!member.active" to remove element from dom.
|
|
||
| # Update existing alerts that have been modified | ||
| alertsToUpdate = _.filter(alerts, (alert) -> | ||
| active_alert = _.filter(kpi.alerts, (kpi_alert) -> kpi_alert.service == alert.service)[0] |
There was a problem hiding this comment.
Rename to existingAlertand use _.find()
| recipient_change = false | ||
| recipient_change = true if recipient_ids.length != active_ids.length | ||
| for i in [0...recipient_ids.length] | ||
| recipient_change = true if recipient_ids[i] != active_ids[i] |
There was a problem hiding this comment.
Lets simplify the change detection above by comparing the arrays like so: "#{[1, 3, 2].sort()}" == "#{[2, 1, 3].sort()}", also I think it should be clear with the variable naming what is existing and what is new. So naming active_ids to existingActiveIds etc.
|
Hi @adamaziz15 will post this here for other reviewers, as discussed in slack if we could also adjust / add these things:
🍻! |
|
@xaun / @adamaziz15 From what I've seen so far I think this won't be a problem, but please verify that there is no backward incompatibility with mno-enterprise (= if you upgrade impac-angular without upgrading mno-enterprise, you can still add alerts with recipients) |
|
@cesar-tonnoir @xaun I made a few changes to make sure there is no issue with backward-compatibility and tested everything. Seems to work with no issues. Please review, should be all set 👌 |
3 participants
No description provided.