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

feat: add top-level agenda.disable and agenda.enable methods #1536

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pdfowler
Copy link
Contributor

I noticed my work was listed as a blocker on #1502, so here is my original implementation of the enable and disable methods ported to the agendaTS codebase.

  • this code has been migrated over from Add top level disable and enable #1109
  • migrates old methods from individual files to the top level Agenda Class
  • test suite updated with original test suite content from original fork

* this code has been migrated over from agenda#1109
* migrates old methods from individual files to the top level Agenda Class
* test suite updated with original test suite content from original fork
@simllll
Copy link
Contributor

simllll commented May 24, 2023

Oh great, thanks for your work.

One thing to mention: I've introduced the JobDbRepository to "abstract" away the datbaes layer,
in your implementation the database querys (.collection.updateMany()) are now implemented directly in the index.ts file, can you refactor it so that you just move the "updateMany" things into the JobDbRepository and call the method instead.
What's the idea behind: have adapter/database logic abstracted away from application/business logic. this helps to easier read and test code, but also gives the possibility to switch the database layer, without changing any application code/logic.

Thanks again for your contribution.

@harisvsulaiman
Copy link
Member

@simllll I love the idea that we could switch out db layer in the future.
But I think we can support many features like (watching for new jobs and even job defs) if we stick to mongo

@simllll
Copy link
Contributor

simllll commented May 24, 2023

@simllll I love the idea that we could switch out db layer in the future. But I think we can support many features like (watching for new jobs and even job defs) if we stick to mongo

Yep, my main concern is to keep "business" logic and adapter logic seperat, this helps maintating a cleaner code :) But it would be a possible "side effect" that allows swichting the layer too, but this is for sure another topic we can discuss in the future ;-)

@pdfowler
Copy link
Contributor Author

pdfowler commented Jun 1, 2023

Took a quick pass at abstracting things out. I like the idea of making it more DB agnostic, but I imagine the transition will be challenging, especially for something like this which is so interconnected with mongo syntax.

I'll try to take another pass at cleaning up the formatting in the next few days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants