diff options
| -rw-r--r-- | CHANGELOG | 2 | ||||
| -rw-r--r-- | app/controllers/projects/notification_settings_controller.rb | 19 | ||||
| -rw-r--r-- | app/models/notification_setting.rb | 13 | ||||
| -rw-r--r-- | app/services/notification_service.rb | 3 | ||||
| -rw-r--r-- | app/views/shared/projects/_custom_notifications.html.haml | 4 | ||||
| -rw-r--r-- | spec/controllers/projects/notification_settings_controller_spec.rb | 19 | ||||
| -rw-r--r-- | spec/models/notification_setting_spec.rb | 25 |
7 files changed, 65 insertions, 20 deletions
diff --git a/CHANGELOG b/CHANGELOG index 1c73e385bd6..3dfb88f212a 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/controllers/projects/notification_settings_controller.rb b/app/controllers/projects/notification_settings_controller.rb index 13b38501ae4..05fe5accc65 100644 --- a/app/controllers/projects/notification_settings_controller.rb +++ b/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 diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index f41b011c0ae..43acb4c5a7b 100644 --- a/app/models/notification_setting.rb +++ b/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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 841912b72c5..369bc874843 100644 --- a/app/services/notification_service.rb +++ b/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 diff --git a/app/views/shared/projects/_custom_notifications.html.haml b/app/views/shared/projects/_custom_notifications.html.haml index 8569c523ef4..4e446fe2d10 100644 --- a/app/views/shared/projects/_custom_notifications.html.haml +++ b/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") diff --git a/spec/controllers/projects/notification_settings_controller_spec.rb b/spec/controllers/projects/notification_settings_controller_spec.rb index c5d17d97ec9..a726f70a64a 100644 --- a/spec/controllers/projects/notification_settings_controller_spec.rb +++ b/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 diff --git a/spec/models/notification_setting_spec.rb b/spec/models/notification_setting_spec.rb index 4e24e89b008..df336a6effe 100644 --- a/spec/models/notification_setting_spec.rb +++ b/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 |