Refactor custom notifications controller code and add specs

CHANGELOG

@@ -45,7 +45,7 @@ v 8.9.0 (unreleased)
   - Remove 'main language' feature
   - Pipelines can be canceled only when there are running builds
   - Use downcased path to container repository as this is expected path by Docker
-  - Custom notification level for projects
+  - Customized notification settings for projects
   - Projects pending deletion will render a 404 page
   - Measure queue duration between gitlab-workhorse and Rails
   - Make Omniauth providers specs to not modify global configuration

app/controllers/projects/notification_settings_controller.rb

@@ -3,28 +3,19 @@ class Projects::NotificationSettingsController < Projects::ApplicationController
   def update
     @notification_setting = current_user.notification_settings_for(project)
-
-    if params[:custom_events].nil?
-      saved = @notification_setting.update_attributes(notification_setting_params)
-    else
-      events = params[:events] || {}
-
-      NotificationSetting::EMAIL_EVENTS.each do |event|
-        @notification_setting.events[event] = events[event]
-      end
-
-      saved = @notification_setting.save
-    end
+    saved = @notification_setting.update_attributes(notification_setting_params)
     render json: {
       html: view_to_html_string("projects/buttons/_notifications", locals: { project: @project, notification_setting: @notification_setting }),
-      saved: saved,
+      saved: saved
     }
   end
   private
   def notification_setting_params
-    params.require(:notification_setting).permit(:level)
+    allowed_fields = NotificationSetting::EMAIL_EVENTS.dup
+    allowed_fields << :level
+    params.require(:notification_setting).permit(allowed_fields)
   end
 end

app/models/notification_setting.rb

@@ -31,6 +31,7 @@ class NotificationSetting < ActiveRecord::Base
   store :events, accessors: EMAIL_EVENTS, coder: JSON
   before_save :set_events
+  before_save :events_to_boolean
   def self.find_or_create_for(source)
     setting = find_or_initialize_by(source: source)
@@ -42,12 +43,20 @@ class NotificationSetting < ActiveRecord::Base
     setting
   end
-  # Set all event attributes to false when level is not custom or being initialized
+  # Set all event attributes to false when level is not custom or being initialized for UX reasons
   def set_events
-    return if self.custom? || self.persisted?
+    return if custom? || persisted?
     EMAIL_EVENTS.each do |event|
       events[event] = false
     end
   end
+
+  # Validates store accessors values as boolean
+  # It is a text field so it does not cast correct boolean values in JSON
+  def events_to_boolean
+    EMAIL_EVENTS.each do |event|
+      events[event] = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(events[event])
+    end
+  end
 end

app/services/notification_service.rb

@@ -524,7 +524,8 @@ class NotificationService
     end
   end
-  # Builds key to be used if user has custom notification setting
+  # Build event key to search on custom notification level
+  # Check NotificationSetting::EMAIL_EVENTS
   def build_custom_key(action, object)
     "#{action}_#{object.class.name.underscore}".to_sym
   end

app/views/shared/projects/_custom_notifications.html.haml

@@ -9,7 +9,6 @@
       .modal-body
         .container-fluid
           = form_for @notification_setting, url: namespace_project_notification_setting_path(@project.namespace.becomes(Namespace), @project), method: :patch, html: { class: "custom-notifications-form" } do |f|
-            = hidden_field_tag "custom_events", "true"
             = f.hidden_field :level
             .row
               .col-lg-3
@@ -21,7 +20,8 @@
                   .form-group
                     .checkbox{ class: ("prepend-top-0" if index == 0) }
                       %label{ for: "events_#{event}" }
-                        = check_box_tag "events[#{event}]", true, @notification_setting.events[event], id: "events_#{event}", class: "js-custom-notification-event"
+                        = check_box("notification_setting", event, {id: "events_#{event}", class: "js-custom-notification-event"})
+
                         %strong
                           = event.to_s.humanize
                           = icon("spinner spin", class: "custom-notification-event-loading")

spec/controllers/projects/notification_settings_controller_spec.rb

@@ -33,6 +33,25 @@ describe Projects::NotificationSettingsController do
         expect(response.status).to eq 200
       end
+
+      context 'and setting custom notification setting' do
+        let(:custom_events) do
+          events = {}
+
+          NotificationSetting::EMAIL_EVENTS.each do |event|
+            events[event] = "true"
+          end
+        end
+
+        it 'returns success' do
+          put :update,
+              namespace_id: project.namespace.to_param,
+              project_id: project.to_param,
+              notification_setting: { level: :participating, events: custom_events }
+
+          expect(response.status).to eq 200
+        end
+      end
     end
     context 'not authorized' do

spec/models/notification_setting_spec.rb

@@ -12,5 +12,30 @@ RSpec.describe NotificationSetting, type: :model do
     it { is_expected.to validate_presence_of(:user) }
     it { is_expected.to validate_presence_of(:level) }
     it { is_expected.to validate_uniqueness_of(:user_id).scoped_to([:source_id, :source_type]).with_message(/already exists in source/) }
+
+    context "events" do
+      let(:user) { create(:user) }
+      let(:notification_setting) { NotificationSetting.new(source_id: 1, source_type: 'Project', user_id: user.id) }
+
+      before do
+        notification_setting.level = "custom"
+        notification_setting.new_note = "true"
+        notification_setting.new_issue = 1
+        notification_setting.close_issue = "1"
+        notification_setting.merge_merge_request = "t"
+        notification_setting.close_merge_request = "nil"
+        notification_setting.reopen_merge_request = "false"
+        notification_setting.save
+      end
+
+      it "parses boolean before saving" do
+        expect(notification_setting.new_note).to eq(true)
+        expect(notification_setting.new_issue).to eq(true)
+        expect(notification_setting.close_issue).to eq(true)
+        expect(notification_setting.merge_merge_request).to eq(true)
+        expect(notification_setting.close_merge_request).to eq(false)
+        expect(notification_setting.reopen_merge_request).to eq(false)
+      end
+    end
   end
 end