Added custom notification setting to dropdown

What does this MR do?

Adds an option in the notification dropdown to allow users to customise their notification settings.

What are the relevant issue numbers?

Part of #12758 (closed)

Screenshots (if relevant)

Added 1 commit:

• 6229df12 - Shows loading spinner when changing notification level

Added 1 commit:

• cf64e333 - Fixed issue with dropdown not replacing correctly

Added 1 commit:

• 59694469 - Removed description

Added 1 commit:

• 59694469 - Removed description

@iamphill Can we get a screenshot?

@DouweM of course!

When anything but custom is chosen

When custom is chosen

The dropdown allows you to toggle between custom and preset notification types.

Modal window

I dont think we need descriptions underneath the checkboxes as they seem pretty self explanatory

mentioned in issue #12758 (closed)

Added 1 commit:

• 2889e29c - Removed method on model, instead get variable straight from model

Unmarked this merge request as a Work In Progress

Reassigned to @jschatz1

Reassigned to @fatihacet

@fatihacet can you do an initial review.

I think we shouldn't pass callback function in this way. It's because this will be the $document inside the method you passed. Usually you want this to point your class itself but with this usage it won't. To fix that in line 14, we are using => in method declaration which is also a bad thing because CoffeeScript compiler does some extra stuff to fix that in behind the scene. See the code below. This is what CS compiler adds to code.

var bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; };

this.NotificationsForm = (function() {
  function NotificationsForm() {
    this.toggleCheckbox = bind(this.toggleCheckbox, this);
  }
})();

Instead of this we can use the code below. By doing that we will always be sure this will point the class and save us from some magic.

initEventListeners: ->
  $(document).on 'change', '.js-custom-notification-event', (e) => @toggleCheckbox e

I don't understand how this helps. Either way we are using a double arrow. Just the way I have done it, it calls the functions rather than calling a function to call another function.

this could never be document because of the fat arrows.

& the way I have done it has been done through different places in the code.

Actually in the current version, CoffeeScript fixes it for us behind the scene. If you would write it in vanilla JS you wouldn't do that so basically we are abusing CoffeeScript here. When you put a fat arrow in the method declaration CoffeeScript creates another bound function for it. What if CoffeeScript compiler changes this behaviour in the next version? It would take hours to find what is wrong and fix it.

Can we use e.currentTarget here? Would it be the same?

Can you move these lines to a method and call it in here? It will increase the readability and code will look better. Right now I have to read the lines below to understand what you are trying to do. Instead it would be easy to just read the method name and understand what it is doing. It's my 2 cents.

@jschatz1 I added a few comments. I will assign back to @iamphill @DouweM can you review the backend part in meanwhile?

Reassigned to @iamphill

Added 1 commit:

• 2c5f343f - Moved beforeSend into a method

Status changed to merged