-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add actions to pods #126
Add actions to pods #126
Conversation
…service-sending-of-actions-request-to-match-api-change
…d-controller-to-trigger-actions
…tions-after-pod-action-trigger
…re/issue-57-provide-visual-feedback-when-pod-actions-busy
…al-and-alerts-approach
…sing a template url
…al-and-alerts-approach
…C-OPENHELPD-23
action_data = data.get('action', {}) | ||
success, payload = pod.perform_action(action_data.get('type'), action_data.get('payload', {})) | ||
if success is True: | ||
case = Case.objects.get(id=case_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For security, filter by org too to make sure a user can never act on a case outside of their org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually come to think of it, it should actually be checking Case.access_level(user)
to make sure this user has correct access rights to this case.
Obviously this applies to anywhere requests can access or modify cases.
…action' into EPIC-OPENHELPD-23
|
||
expect(document.querySelector('.modal-body').textContent) | ||
.toMatch('Are you sure you want to do this?') | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rowanseymour I ended up modifying the approach we discussed a bit for modals. Is it supporting both the prompt-strings approach and the template url approach. What ended up differently though, was that instead of providing a prompt template url, one can provide a template url for the entire modal. The reason for this is that it gives us more flexibility for controlling how the modal looks. Thought I should bring this up since it is a slight change in what we discussed.
@rowanseymour Ready for review. |
@@ -573,12 +588,12 @@ controllers.controller('CaseController', ['$scope', '$window', '$timeout', 'Case | |||
#---------------------------------------------------------------------------- | |||
|
|||
$scope.onWatch = () -> | |||
UtilsService.confirmModal("Receive notifications for activity in this case?").then(() -> | |||
UtilsService.confirmModal("Receive alerts for activity in this case?").then(() -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this change? Seems to me alerts and notifications are different things.
I think I might have mentioned this on the dev list but I think at some point it would make sense to surface notifications in the UI. Should also ensure that pods have the ability to generate notifications and control how they are rendered as emails.
Also can we replace these calls with ModalService.confirm(..)
calls so we're certain that new service can replicate the existing functionality, and we don't have two confirm modal methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, this was an accidental replace from a search-and-replace, sorry! Will fix.
Also can we replace these calls with ModalService.confirm(..) calls so we're certain that new service can replicate the existing functionality, and we don't have two confirm modal methods.
Can replace these calls here, but I'm a bit worried about replacing all confirm modals in this PR, get the feeling it will make this PR's scope grow quite a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but you have to promise to change them all soon :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/me nods
I'll give replacing them a try in this PR to get a sense of the level of effort required, will see from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took a peek and there's only ~15 calls I think - but some things you'll need to add/check:
- a danger param which sets the styling of the OK button to danger
- that prompt strings can include markup, e.g.
"Apply the label <strong>" + label.name + "</strong> to the selected messages?"
- or there's at least an easy way to do that without creating an entire modal template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look. I've created #127 for this, will make this change as a separate PR.
@rowanseymour I think the review comments so far have been addressed |
…cter-limits OPENHELPD-128 - FAQ character limits
This allows the pods to specify a list of actions, which are displayed as buttons, which perform actions through callbacks to the pods.