Mattermost-server: Clean up and add thorough tests to app/notification.go

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide, specifically for machine setup and for developer workflow.

---

Notes: Jira ticket

Clean/split up the sendNotification and other large functions in app/notification.go into smaller, more testable functions. Then add tests for these functions to throughly test as many cases as possible.

Goal here is to reduce the chances of a bug being introduced into the complex and critical notification code paths by reducing complexity and thoroughly testing with unit tests.

[ghost]

I would like to try this one, if nobody else is already working on it.

[amyblais]

Thank you @pchch! Let us know if you have any questions.

[hanzei]

Hey @ghost

How are doing with this ticket? Do you have any questions about this ticket? Feel free ask them here or on https://community.mattermost.com.

[nitishm]

@henzai Is this something a beginner to mattermost could tackle ? I haven't spent too much time in the codebase but am pretty well versed in Go.

If so I would like to take this on, as a way to get familiar with the server code.

[hanzei]

Hey @nitishm,

Thanks for your intensest in this issue and sorry for the late reply. I was on vacation.

This issue is a good introduction into the mattermost code base. Would you like to work on this?

[nitishm]

@hanzei Sure would !

[hanzei]

Great! Let me know if you have any question.

[hanzei]

Hey @nitishm,

How is the work on this one going? Can I help with anything?

[nitishm]

@hanzei I apologize for the late response. I have been really caught up with work lately. I might have to pass this on.
Thanks.

[hanzei]

Hey @nitishm,

Thanks for your response. Let's see how the next days are going. If you want to pass on this one, just let me know.

[hanzei]

Making this one available for the public again due to inactivity.

[giorgosdi]

I'd like to give it a Go (🥁) if no one is working on it 😃

[hanzei]

Go :drum: ahead, thanks for looking into this @giorgosdi. :+1:

[giorgosdi]

@hanzei I assume you mean me 😂

I created a pull request which is very much still in progress, but it's a good way to track changes.

https://github.com/mattermost/mattermost-server/pull/10492

if you want, have a look and comment on any discrepancies that you spot as its my first time with Go.

Thanks

[enahum]

Re-opening this issue as I reverted the PR cause it was causing a server panic.

RevertPR here #10895

[hanzei]

Hey @giorgosdi,

Sorry, that we had to revert your PR. This should have been caught in the review process. Would you be open on re submitting the PR? Looking forward to this.

[giorgosdi]

Hey @hanzei no problem ! Yeah i would like to continue this work. I already spoke with @jespino on how to proceed so we can merge code without any problems

[hanzei]

@jespino Is this fixed with https://github.com/mattermost/mattermost-server/pull/11047?

[jespino]

@hanzei not really, it will be a serie of small PRs improving the quality there. Probably @giorgosdi will know when we can close this ticket.

[giorgosdi]

@hanzei yeah sorry about the late of my response. So as @jespino mentioned it will be a series of PRs. The code is more or less there, I just need to find some time to do it 😬

[hanzei]

@giorgosdi Are you still working on this issue?

[giorgosdi]

Hey @hanzei , nope I left it aside due to other things, sorry 😕

[hanzei]

No problem. Thanks for the time you spend on this issue.

[fedealconada]

@hanzei I could take this one. Since @giorgosdi has previously worked on this (a year ago approx) and he has even submitted a PR which was reviewed and approved (but later on reverted), what I could do is to take what he's done + try to update that code (because notification.go has been changed since the moment of the PR) + complete that with the WIP things that @giorgosdi mentions on his PR description and add some other things I would see to improve and clean the code. Thoughts?

[hanzei]

That would be awesome @fedealconada :tada:

[fedealconada]

Great! I'll work on it :)

[isacikgoz]

This ticket will be done internally by the server team. Thanks everyone for putting a lot of effort onto this.