Skip to content

Reduce amount of property definitions in ChatNotificationService

Peter Leitzen requested to merge pl-fix-chat-notification-service-ee into master

What does this MR do?

Before this commit we redefined properties for supported events on each new instance of ChatNotificationService during "runtime".

As a result from these redefinitions we saw warnings like:

bundle exec rspec --warnings spec/models/project_services/chat_notification_service_spec.rb
1 | grep "eval.*warning: previous"

(eval):8: warning: previous definition of push_channel= was here
(eval):14: warning: previous definition of push_channel_changed? was here
(eval):18: warning: previous definition of push_channel_touched? was here
(eval):22: warning: previous definition of push_channel_was was here
(eval):8: warning: previous definition of issue_channel= was here
... many more ...

To reproduce from the Rails console do

rails console
$VERBOSE = true
ChatNotificationService.new
ChatNotificationService.new

This commit defines the properties only once during "compile time".

This MR also improves ~performance as redefining methods (using class_eval) over and over again is slower than not doing it.

This idea came up during a code review for https://gitlab.com/gitlab-org/gitlab-ee/issues/13203

Just making sure we're not breaking EE.

CE is at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32363

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Peter Leitzen

Merge request reports