Wrong id used on remove

[dfliess]

Hi @darkterra

Thanks for you efforts and good work. I found that findByStorageId and remove documentation states:

The id searched (remember, this id is not the event itself id)

but on code remove is doing

    if (objectId.isValid(id)) {
        query._id = objectId(id);
    }

and findByStorageId

    const lookup     = { 'storage.id': id.toString() };

When fixing, doc or functionality, please consider comments on https://github.com/darkterra/mongo-scheduler/issues/4

[darkterra]

Hi @dfliess !

Thank you so much :)

You right, I made an mistake when I write the doc. But now it's Fix (and I add a new parameter to "remove" with the "id of the event" it self) !

Can you update the module (2.2.0), try, and confirme me it's ok ?

[dfliess]

Great! also maybe you can do something like remove by id or query ? I'm thinking that, although might be improbable, someone can have same Ids for different collections. Now I'm doing something like:

scheduler.listAsync({ query: { 'name': eventName, 'storage.collection': colName, 'storage.id':  objId }})
  .each(event => scheduler.removeAsync({ id: event._id.toString() }))

(yes using bluebird promisify)

On another subject, I may suggest to use a better Event emmiter or Dispatcher, or maybe allow to receive an EventEmmiter implementation, with the purpose of allowing namespaces and wildcard listeners.

[darkterra]

Great! also maybe you can do something like remove by id or query ? I'm thinking that, although might be improbable, someone can have same Ids for different collections.

I'm not sure I understand... The module only uses its own _(scheduledevents) collection.

The remove method only works for the documents created within the module's collection. Currently you can delete an event recorded in the database before it is triggered by its name, its own id, the id you thought about when creating the event or the "after" property.

What is the point of being able to create a query (to delete the event)?

On another subject, I may suggest to use a better Event emmiter or Dispatcher, or maybe allow to receive an EventEmmiter implementation, with the purpose of allowing namespaces and wildcard listeners.

Very good idea !

I'm not up to date on EventEmmiter, do you have some leads to help me find a good solution? The implementation of an EventEmmiter is a cool idea too!

---

Thank you for your very interesting feedback! You can if you wish to propose new features by pull request.

And if you like the module, do not hesitate to click on the star (right next to the fork at the top of the page ;) )

[dfliess]

hi @darkterra

I mean that someone may have something like storage.collection: 'users', storage.id: 1. and another event storage.collection: 'payments'. storage.id: 1. so the key is compound.

About the event emitter implementation, there are several implementations, like https://www.npmjs.com/package/eventemitter2 or https://www.npmjs.com/package/eventemitter3 or https://www.npmjs.com/package/dispatcherjs with their pros and cons each one. That's why I was thinking on allowing injection of the emitter on the constructor.

[darkterra]

Hello @dfliess,

To perform this type of operation, you can do it in two steps:   - Use the scheduler.list method with your parameter query (https://github.com/darkterra/mongo-scheduler/blob/master/lib/scheduler.js#L262)   - Then you can use the id of the event itself to pass them to the method scheduler.remove Exactly as you do in your example (but normally, you not obligated to cast the id into string).

The reason I do not allow deleting directly by query is that, if the wrong query was dimenable, then there will be edge effects. Doing in two steps allows the user of the module to easily check and even select other criteria (if necessary) before deleting

---

For your proposal to modify the event emmiter, I prefer to keep the native version (this avoids me to have an additional dependency) but, I will add the possibility to include a custom event emmiter (as you also propose), the only condition is that it must be compatible with the native version of the event emmiter.

I will work on it in a few days.

[darkterra]

Hello @dfliess,

Finally, after a year and a half, I took the time to set up this feature ! Sorry for the wait, I had other personal priorities and I also needed to rewrite a lot of the module to be able to easily implement this feature.

This feature will certainly be the only one that will not be subjected to unit tests, if the module is still useful to you, I invite you to do your own tests and come back to me if there is a problem :)

Try the last version (https://github.com/darkterra/mongo-scheduler/releases/tag/v2.4.0) :)