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

Email reminder on workflow steps #131

Merged

Conversation

veggiematts
Copy link
Contributor

Note: this PR is based on PR #117 and therefore contains all of its commits.

This patch allows to trigger an email reminder after a resource step is assigned to a user.
The delay (day-based) can be configured for every resource step:

mail_reminder

@benheet
Copy link
Contributor

benheet commented Nov 16, 2016

@veggiematts can you say more about what the reminder delay is doing? Is it sending a reminder email to the assigned person x number of days after the workflow step is activated? I don't think the workflow template is the proper place to set email reminders that go to a specific person or group. I'd enable a workflow reminder as a global feature that went to anyone assigned to any active workflow step X days after the step was active (if not yet complete) OR i'd leave it to the individual to select whether they wanted reminders somewhere within the My Queue page. This implementation of a reminder feature seems more complex then it needs to be and I'd rather not see this merged without further discussion.

@veggiematts
Copy link
Contributor Author

It sends a reminder email to the assigned persons (each member of the group assigned to this step) n days after the step has been started, where n is the reminder delay.

I thought the specification was discussed with the community prior to this implementation (in the coral-admin mailing list?)

Maybe @xsong9 can tell us more about what was discussed?

@xsong9
Copy link
Contributor

xsong9 commented Nov 17, 2016

@benheet The email reminder can be enabled in the Edit the Workflow window in the Routing tab associated to a resource. It's part of the local workflow enhancement implementation #117 . Setting a workflow reminder as a global feature will not work because the number of delay days for a step varies depending on a specific resource. For example I have a workflow setup for ebook package acquisition and it includes three steps: licensing, invoicing and setup access. I can use the same workflow for both backfile purchase and frontfile 2017 purchase. The backfile access can be set up right after the purchase, but the frontfile access need to wait until the titles are available, which means a couple of months later or even longer. With this implementation, I can set up different dates for the access step for the backfile and the frontfile locally. It provides flexibility and can avoid creating separate workflows in the Admin.

Please let me know if you have more concern or question about this feature.

@benheet
Copy link
Contributor

benheet commented Nov 17, 2016

Note this PR contains the commits from #117 but I have reservations about the additional alert features added in this PR. I propose that we return to and merge PR #117 and deal with the additional functionality of this PR later.

@benheet
Copy link
Contributor

benheet commented Dec 13, 2016

@veggiematts @xsong9 this pull request has been approved but it has conflicts that need to be resolved before it's merged. Can you take responsibility for resolving the conflicts please?

@benheet benheet removed the question label Dec 13, 2016
@jeffnm jeffnm added this to the Version 2.0.0 Beta milestone Dec 13, 2016
@jeffnm
Copy link
Member

jeffnm commented Dec 13, 2016

I've added the 2.0 beta milestone. If we can get the conflicts resolved in time, we can include it. If not, I'll remove the milestone.

veggiematts and others added 13 commits December 14, 2016 09:24
 - Display library as a multiple select list
 - Keep only "Approved" and "Need approval" acquisition types in the demo form
 - Add a summary after resource is added
 - Allow to submit fund without cost and cost without fund
 - Start workflow when available
 - Update client form (textareas)
 - Default values for enterNewWorkflow
 - Add Email class
 - License Type Note should be in acquisitions tab.
Config: make the acceptance tests point to localhost:8000
See TESTS.md for how to set it up.
Also, extraction of login to it's own test and load a cookie before
each test to not have to login and set the language.
connordelacruz and others added 16 commits December 14, 2016 09:24
… failing, rather than waiting the default amount of time.
… to test using this framework. Something to do with the way file uploads are handled in the module.
 - Remove dependancies from Coral
Added more wait for page to be ready steps to the tests to ensure slow loads don't introduce intermittent failures, such as those I was seeing.
 - Use the Fund table for ResourcePayments
@veggiematts
Copy link
Contributor Author

Conflicts resolved on forked branch, but it somehow doesn't seem to be updated in this PR.

@veggiematts
Copy link
Contributor Author

Don't merge into master yet, there are still issues in this branch.

@veggiematts
Copy link
Contributor Author

Nevermind, the problem was on my coral instance. As far as I'm concerned, we're good to go!

@jeffnm
Copy link
Member

jeffnm commented Dec 14, 2016

There aren't any conflicts any more, but I'm seeing a bunch of file changes that should already be in coral-erm/master. For example, all the of the Test Suite files are showing up in this pull request. I guess that's not a problem since the end result will be the same, but it strikes me as odd.

@veggiematts
Copy link
Contributor Author

@jeffnm Do you know what could have caused this?

@jeffnm
Copy link
Member

jeffnm commented Dec 20, 2016

@veggiematts I don't. Perhaps something in the way the rebase was done?

Do you think it's a problem?

@veggiematts
Copy link
Contributor Author

@jeffnm I don't know but I didn't like that either. Here is a new push with a clean diff.

@jeffnm
Copy link
Member

jeffnm commented Jan 10, 2017

@veggiematts beautiful. I'll merge it.

@jeffnm jeffnm merged commit 13d1120 into coral-erm:master Jan 10, 2017
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.

6 participants