Add SMS and email communications handling

[pwalsh]

See #17 (comment)

[codecov-commenter]

Codecov Report

Merging #17 (3d6e899) into main (2260410) will decrease coverage by 4.28%.
The diff coverage is 80.64%.

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
- Coverage   93.39%   89.11%   -4.29%     
==========================================
  Files          40       49       +9     
  Lines         742     1001     +259     
  Branches       35       57      +22     
==========================================
+ Hits          693      892     +199     
- Misses         49      109      +60     

Impacted Files	Coverage Δ
src/core/staff-users/routes.ts	100.00% <ø> (ø)
src/migrations/00_initial.ts	76.00% <ø> (ø)
src/sms/index.ts	52.63% <52.63%> (ø)
src/core/staff-users/models.ts	80.00% <57.14%> (+2.22%)	⬆️
src/email/index.ts	57.14% <57.14%> (ø)
src/core/open311/repositories.ts	58.33% <60.00%> (ø)
src/core/communications/helpers.ts	69.56% <69.56%> (ø)
src/core/communications/repositories.ts	71.87% <71.87%> (ø)
src/migrations/04_communications.ts	81.81% <81.81%> (ø)
src/core/communications/event-handlers.ts	83.33% <83.33%> (ø)
... and 26 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2260410...3d6e899. Read the comment docs.

[pwalsh]

@amirozer hey. I'm not merging this directly as it is extensive enough to require review, and, there are some open issues.

First up, this issue describes the logic and implementation #5 (comment)

I believe everything discussed there is covered, but we should check. Also, the notes I made in #5 (comment) are required to finalise this PR, in quick summary:

1. Multiple status stages which map to "closed" - I did this, see Multiple status stages which map to "closed" #12 and https://github.com/govflow/govflow/blob/feature/communications/src/core/service-requests/models.ts#L15
2. Will we assume closed service requests cannot reopen
3. Our discussions to date around admin users has been about "admin users for the system", and not "admin users who are staff users for a given jurisdiction" - the code currently assumes staffUser.isAdmin returns true if the user in an admin. Let's discuss if we need to change that.

Another point: I put the event emitters in the repository methods. They should be in the route handlers after repository methods are called, so they are uncoupled with the data access and storage implementation. I did this now as it means I had access to logic about what changed, which is used in dispatching a message, but on reflection it should be pretty straight forward to get that minimal logic from the request and the response of any repository method calls.

@amirozer also don't miss the README which explains some important things https://github.com/govflow/govflow/blob/feature/communications/README.md#communications

[amirozer]

I want to make a clear separation of repositories from other higher level services. Open311 is not really a repository as it uses two actual repositories - for services and for service requests - but uses a different contract to align with the Open311 standard.

In the meantime I kept the repository as it was originally called and left the declarations together with the other repositories, but this should change in the future.
However I did add the Open311 types to a separate file, to distinguish it, as it also doesn't describe models. So I prefer to keep that file in place and not combine it into the global types file which declares types used by multiple services.

[pwalsh]

@amirozer hey, whether open311 is a repository or not is orthogonal to the change here, where I just made sure that all types that describe data (as I added a bunch of missing ones) are in types/data (if the data comes from a model or not also seems orthogonal to me). I think discussion about naming and design (repositories, services) can be done in issues dedicated to that so we can get a shared understanding so the code evolves towards that clearly and purposefully.

Is the moving of those type definitions from open311/helpers into types/data a change you want me to revert in this PR? Or something we do later as part of a general design cleanup as applicable?

[amirozer]

Yes I'd like this specific change to be reverted. Let's talk about this when we meet at the office

[pwalsh]

@amirozer cool, it is in b3cd0fd and also please see #25 which is about naming of interfaces, for us to discuss.

[amirozer]

Not a blocker for this PR, but a general note about repositories: I aim to use repositories for data access only. So you have the DB models, on top of that you have a repository that implements an interface and can be swapped with another one so we work with a different DB/external API, and on top of that you have services using those repositories.

So in this case, the dispatch functions belong to a service, and only findByServiceRequestId should be in the Communication repository.
Also, the dispatch functions receive a Mongoose model instead of a repository, so they are coupled with Mongoose. They should use the repository, and the repository should implement a create function that will use the model.
That way, one can switch the way Communications are stored, without implementing all the dispatch functions.
Also we can in the future allow registering your own implementation for services and not only repositories, so one could replace how messages are sent without replacing the way Communications are stored.

Makes sense?

[pwalsh]

Hey @amirozer again, we probably should discuss and design in advance so we have a shared understanding of the boundaries of what a repository is/isn't etc.

Maybe write up an issue for how you think we should use repositories, and or services, and any other design patterns, so we can discuss?

My preference would be that we don't go too heavily into java/c# type abstractions/layers, but I don't mind as long as we discuss in advance and do so based on a common understanding of the boundaries of abstractions we use, etc.

In any event, In the case of this PR:

I aim to use repositories for data access only

So, sure, but, the sms/email providers are external data APIs - this will become even more explicit when we add features later like management of unsubscribes etc. I think what you really mean is that a single repository shouldn't abstract over multiple data backends(?), in which case, guilty :).

So in this case, the dispatch functions belong to a service

Well, they belong to a "service" if our code has the concept of a service, but, it doesn't. We can discuss how we use a service layer in the future, though.

Also, the dispatch functions receive a Mongoose model instead of a repository, so they are coupled with Mongoose. They should use the repository, and the repository should implement a create function that will use the model.

Yes, the are coupled to Sequelize because they are implemented on the repository and the codebase does not have a separate service layer.

Makes sense?

Yes, I see the direction you are thinking in, but, we've never discussed it, so, I think we should do that either in issues on the tracker dedicated to the design of the codebase, or, in some meetings in person which we then follow up by documenting. In any event, these notes/discussions will get lost as comments on the PR.

[pwalsh]

@amirozer thanks for the review. There are two major points I raised in #17 (comment) which you didn't respond to, and I want to make sure we are in sync on them before we merge, being (i) multiple stages mapping to closed, and use of isAdmin as a property on StaffUser