I have reviewed your code to make it a submodule of workflow module.
Please see attached file.
See also the todo statements in the code.

CommentFileSizeAuthor
workflow_notifications_20180108.patch60.55 KBjohnv

Comments

johnv created an issue. See original summary.

saranya ashokkumar’s picture

saranya ashokkumar commented

@johnv,

As soon as possible, I will update all your changes and let you know.

saranya ashokkumar’s picture

saranya ashokkumar commented

I have checked your patch and It looks fine. Thanks for your suggestion.

I have made some changes in routing.yml. Added param Converter for notification urls.

    parameters:
      workflow_type:
        type: entity:workflow_type

Because, Form workflow_url_get_workflow() throwing error for workflow config pages Recoverable fatal error: Object of class Drupal\workflow\Entity\Workflow . So, I set Common parameters for all pages.

Thanks!

saranya ashokkumar’s picture

saranya ashokkumar commented

@johnv,

I have created issues for your suggestions. I will update my status in these below issues queues. Please review.

@todo: use WorkflowScheduledTransition::loadBetween();
@todo: use DEFINE CONSTANT.
@todo: add validation for email adresses.
$todo: use $transitions as parameter, and derive $trigger from that.
@todo: hide below code in WorkflowNotification::loadMultiple()
@todo: use $notifications[] = WorkflowNotification::LoadByProperties()
@todo: use $transition->isScheduled() instead? Or use a Factory.
@todo: next line has no effect.
@todo: the selection upon role is not working.
@todo ->condition('roles', $role, 'IN')

johnv’s picture

johnv
Primary language Dutch
commented

Nice,

if you have any problems with the workflow core methods/functions, I am happy to update them.
Also for example, the WorkflowScheduledTransitions::LoadBetween(), does not support the from_sid and to_sid filters. I can add that.

I also factored some functions and created a 'Trait', so there is less code. I guess you have found in in the code. (you need to use my latest dev version.)
What was your plan? To keep this as a separate module, or as a submodule of Workflow?

I guess a separate module is nicer. Also because we can incorporate the SMS notifications from Workflow SMS Notification
There is lots of features that can be enhanced in this field, so a separate module might be better.
See also https://www.drupal.org/project/scheduled_message

I'd be happy to be co-maintainer or let you co-maintain the main Workflow Module.
Your code looks very fine, perhaps we can make workflow_notifications.module file smaller by adding a helper class.

johnv’s picture

johnv
Primary language Dutch
commented

We may consider re-using the reknowned module Message. See the Message Stack documentation.

johnv’s picture

johnv
Primary language Dutch
commented

Oh, and I did review and test the complete module, except for the scheduled updates.

saranya ashokkumar’s picture

saranya ashokkumar commented

Hi johnv,

I have added you as co-maintainer for workflow notification and I am also happy to be a co-maintainer for main workflow module.

Separate module is okay for me. I am ready to work on whatever you mention in comment #6. I will check all the link you mentioned above and let you know.

saranya ashokkumar’s picture

saranya ashokkumar commented

@johnv,

Didn't get your point in comment #8. can you explain it?

johnv’s picture

johnv
Primary language Dutch
commented

I meant that my review was not complete yet.
The hook_cron part was not tested by me.

saranya ashokkumar’s picture

saranya ashokkumar commented

Hi johnv,

In comment #6, "we can incorporate the SMS notifications from Workflow SMS Notification" - will add the SMS notification features to this module or will use that module as dependent by calling API?

johnv’s picture

johnv
Primary language Dutch
commented

Since the SMS notifications needs additional modules (SMS), it seems best to me that we create a submodule, as part of this module. Then the separate module workflow_sms_notifications can be removed.
See how workflow moduel works.
I found that is better maintainable in 1 module (main module + submodules).

saranya ashokkumar’s picture

saranya ashokkumar commented

Created Issue for SMS Notifications Add SMS notifications Feature.

saranya ashokkumar’s picture

saranya ashokkumar commented

@johnv,

I almost completed the sms notifications module as submodule, within two days I will update the status.

johnv’s picture

johnv
Primary language Dutch
commented

ok.

saranya ashokkumar’s picture

saranya ashokkumar commented

Hi johnv,

Added SMS notification feature. please review!

johnv’s picture

johnv
Primary language Dutch
commented
Status: Active » Fixed

Hi, It has been a long time!
I added many changed to the 3.0.x code base in [#i3534202] and issues from the same date.
Can you check and create a new version?
ITMT I will go through the issue queue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.