SD-216: send notification to project team members when volunteer appl…

[mitchellwrosen]

…ications are submitted

[wolftune]

Yay!! I'm really so happy to see this. I just tested it locally, and it all works!! It's been frustrating to have someone come by to volunteer and I miss them. Thanks so much.

From skimming the code, that looks fine too (although not my area to weigh in) and glad to see some clean-ups along the way! Maybe @singpolyma can do a more knowledgeable quick review of the code.

Btw, picky recommendation that doesn't affect this PR per se: we now recommend doing all work on feature branches and keeping your master matched to the main master.

One issue:

I couldn't easily test because of only one project in the devDB, but the code makes it appear that this probably notifies all users who are team-members of any project even if the application is not for one of their projects. It really should only happen for team-members of the same project that the application was for (and only show the notification preference for projects where the user is a team member). Can you verify the situation in this new code?

(To be clear, the comment-approval notification for moderators is itself put in the universal preferences and the "user notifications" currently, but that's actually wrong. Both this new notification and that moderator one should be project-specific. They should be both be only shown and implemented within projects that the user has the appropriate role for)

[mitchellwrosen]

I couldn't easily test because of only one project in the devDB, but the code makes it appear that this probably notifies all users who are team-members of any project even if the application is not for one of their projects. It really should only happen for team-members of the same project that the application was for (and only show the notification preference for projects where the user is a team member). Can you verify the situation in this new code?

Whoops, good catch; I should use a userIsTeamMemberOfDB :: UserId -> ProjectId -> DB Bool rather than userIsTeamMemberDB :: UserId -> DB Bool. I'm surprised that there's even a userIsAdminDB function, as admins are project-specific too, no?

[wolftune]

indeed, userIsRole type things should all be project-specific. Being established or not is not a role, so that one stays universal, but all the role things, including Admin, should be per-project.

We can and should have a userIsSiteAdmin sort of thing as well though. We'll probably clean this all up when we get better project planning and sort through things more systematically (which we're getting started on already, doing that is top priority). For now, there's this idea that a particular project is the "site project" and the admin of that project is considered a site admin. I'm not sure whether that makes sense really or whether we should just have a totally separate site-admin permission. None of that is directly relevant to this PR's scope though.

So, my opinion is that this should be merged as soon as we know that the volunteer-app notification is at least project-specific in checking for team-membership and that the code is reviewed as good. We can make notes about fixing the related things later.

That said, @mitchellwrosen you'd be welcome to make the same per-project fix for comment-approval notifications so the code for that and for volunteer-app notifications is matched (but that would entail moving that notification to being a project notification instead of user notification). (There's no admin-related notifications yet to worry about)

[mitchellwrosen]

I already made the project-specific team member change

[wolftune]

@mitchellwrosen ah I see, I suppose you're force-pushing to keep the commit history clean…! Well, I'd like this merged ASAP then, deferring to the code experts as long as they're happy :)

Thanks again!!

[chreekat]

thanks :)