[feature] Added disabling of notifications by type, organization and medium

[pandafy]

Closes #5

[nemesifier]

I left some comments below, looking forward to see the implementation to honor the notification settings.

[nemesifier]

NotificationSettingAdmin > NotificationSettingsInline

[nemesifier]

use User.objects.iterator()

[nemesifier]

I am not loving these names, what about names like create_settings_for_ and delete_settings_for_?

[pandafy]

Those were looking long, ns is short for notificationsetting

[pandafy]

I added a migrate command in travis.yml and build does not fail for sample app. The culprit is a notification which is being registered in ready function of SampleNotificationsConfig. This triggered task for creating notification setting, but migrations are not run yet, therefore we were getting error. Can we keep the migration command?

I tried using post_migrate signal for registering notifications, but then we can not register notifications from anywhere in code.
We will be restricted to apps.py and it will require us to refactor current tests.

[nemesifier]

I added a migrate command in travis.yml and build does not fail for sample app.

Does not fail or does fail?

[nemesifier]

what is default type?

Can you make sure the notification types have a huaman readable label instead of things like "connection_is_working"?

[pandafy]

Does not fail or does fail?

The build is not failing when we run migrations on database before running tests. And I was wrong, the error is still happening even when migrations are run before tests. The difference is the error are popping out in celery worker and not in main thread.

The only thing I don't understand that we do almost identical work in device/apps.py and monitoring/apps.py in openwisp-monitoring and there I don't see any error even in celery worker. Is there anything I am missing here?

what is default type?

It is the default notification type which comes with openwisp-notifications. We can un-register it in openwisp-monitoring if we don't want it.

[nemesifier]

I'm experiencing an issue when running migrations backward in order to go back to the previous state and test other PRs, the 0004 migration cannot be unapplied on my system, can you please try it as well?

$ ./manage.py migrate notifications 0003

Operations to perform:
  Target specific migration: 0003_default_permissions, from notifications
Running migrations:
  Rendering model states... DONE
  Unapplying notifications.0004_data_migration...Traceback (most recent call last):
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/sqlite3/base.py", line 396, in execute
    return Database.Cursor.execute(self, query, params)
sqlite3.OperationalError: no such table: openwisp_notifications_notificationuser

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/core/management/__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/core/management/__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/core/management/base.py", line 328, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/core/management/base.py", line 369, in execute
    output = self.handle(*args, **options)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/core/management/base.py", line 83, in wrapped
    res = handle_func(*args, **kwargs)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/core/management/commands/migrate.py", line 233, in handle
    fake_initial=fake_initial,
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/migrations/executor.py", line 121, in migrate
    state = self._migrate_all_backwards(plan, full_plan, fake=fake)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/migrations/executor.py", line 196, in _migrate_all_backwards
    self.unapply_migration(states[migration], migration, fake=fake)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/migrations/executor.py", line 269, in unapply_migration
    state = migration.unapply(state, schema_editor)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/migrations/migration.py", line 175, in unapply
    operation.database_backwards(self.app_label, schema_editor, from_state, to_state)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/migrations/operations/special.py", line 196, in database_backwards
    self.reverse_code(from_state.apps, schema_editor)
  File "/home/dev/Code/openwisp/openwisp-monitoring/openwisp_monitoring/notifications/migrations/0004_data_migration.py", line 32, in undo_migrate
    mtr_notification_user.objects.bulk_create(notification_user_qs)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/models/query.py", line 469, in bulk_create
    if not objs:
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/models/query.py", line 280, in __bool__
    self._fetch_all()
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/models/query.py", line 1261, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/models/query.py", line 57, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 1152, in execute_sql
    cursor.execute(sql, params)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/utils.py", line 100, in execute
    return super().execute(sql, params)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/utils.py", line 68, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/utils.py", line 77, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/home/dev/.virtualenvs/openwisp2/lib/python3.7/site-packages/django/db/backends/sqlite3/base.py", line 396, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: no such table: openwisp_notifications_notificationuser

[nemesifier]

Rebase this on the current dev please.

[pandafy]

@nemesisdesign "All" organizations created a confusion, Actually the whole purpose for adding an "All" setting was to allow notifications for devices(target objects) without organizations. But since notifications are only sent to superusers if target organization is not present, I have removed the option of "All" from organization users.

[nemesifier]

1. NotificationSetting objects created for OrganizationUser(is_admin=False)

Why are notification settings created for non staff users?
These settings should be created only for staff users which are also admin of an organization.

If a user is created with is_admin=False in the OrganizationUser, they should not receive notifications nor have settings enabled.

In the future, we will support notifications designed for end users, but for now, all the notifications we're creating are for network administrators, which will only be staff users with OrganizationUser(is_admin=True).

2. All vs org settings

I came to the conclusion that this may be too much, I think you also have this impression.

It's not clear for example, if notifications are enabled in ALL but not enabled in the org, which setting overrides which?

It may not be worth continuing to pursue this path.
Maybe we can simplify have one setting for each org the user is member and has is_admin=True and give up with the "all" setting.

3. Checking vs Unchecking, WTF?

Look at this, I disable the web notifications for PING problem, save, and also the email notification gets unchecked.. this shouldn't happen:

Or is it on purpose? If it is, we should rather do it with javascript to make it clear from the UI.

Look at this other one:

Basically, if the web notification is unchecked, also the email notification gets unchecked?
It may be a sensible thing to do, but we need to make it clear for the user otherwise they'll be super confused. One of the possible ways we can do this is to use JS to automatically uncheck the email notification checkbox if the related web notification gets unchecked. But if we do this, we have to do the opposite as well: make sure checking the email notification also checks the web notification.

[nemesifier]

return '{type} - {organization}'.format(
    type=self.get_type_display(),
    organization=self.organization if self.organization else _('All'),
)

[nemesifier]

Notification Preferences

[pandafy]

1. NotificationSetting objects created for OrganizationUser(is_admin=False)

Fixed this one. added user__is_staff as well in the query.

2. All vs org settings

I think All settings was poorly implemented or the was misleading. The goal of providing an all setting was to allow super users control notifications which does not belong to any organization i.e., a target object which does not belong to any organization. I did not provide an option to override settings because I did not aimed for it. Since, notifications which we are dealing now are created for Device object which has organization as required field, so it does not matter much to have control over such notifications. But, it will be a nice to have feature, we might need to think about accessibility for user and implement it accordingly. What do you think?

3. Checking vs Unchecking, WTF?

I should have seen this coming. Fixed it using JS. I have also retained the logic in models since these settings can also be modified from API.

I also caught an edge case during fixing these.

• When an organizationuser is modified, say it's is_admin or organization attribute is updated,

Also fixed this!

[nemesifier]

1. NotificationSetting objects created for OrganizationUser(is_admin=False)

Fixed this one. added user__is_staff as well in the query.

Great, see also #113

I found that today.

1. All vs org settings

I think All settings was poorly implemented or the was misleading. The goal of providing an all setting was to allow super users control notifications which does not belong to any organization i.e., a target object which does not belong to any organization. I did not provide an option to override settings because I did not aimed for it. Since, notifications which we are dealing now are created for Device object which has organization as required field, so it does not matter much to have control over such notifications. But, it will be a nice to have feature, we might need to think about accessibility for user and implement it accordingly. What do you think?

For sure, if we need it, we'll invest time in finding a good way to implement it.

1. Checking vs Unchecking, WTF?

I should have seen this coming. Fixed it using JS. I have also retained the logic in models since these settings can also be modified from API.

I also caught an edge case during fixing these.

• When an organizationuser is modified, say it's is_admin or organization attribute is updated,

Also fixed this!

Great, works a lot better now.
I think it's ready to merge.
I'll keep opening issues for the problems I will find during testing in the real world.