Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifications system for Evans #163

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
6 changes: 5 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class ApplicationController < ActionController::Base
include CustomPaths
helper CustomPaths

before_action :set_time_zone
before_action :set_time_zone, :set_notifications_for_current_user

helper_method :can_edit?, :logged_in?, :admin?

Expand Down Expand Up @@ -41,4 +41,8 @@ def require_admin
def set_time_zone
Time.zone = 'Sofia'
end

def set_notifications_for_current_user
@notifications = Notification.unread_for_user(current_user) if user_signed_in?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Повтарям: такива глобални instance variable-и са гадни. Замени го с:

helper_method :notifications

def notifications
  @_notifications ||= Notification.unread_for_user(current_user) || []
end

end
end
10 changes: 10 additions & 0 deletions app/controllers/notifications_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class NotificationsController < ApplicationController
def index
end

def show
notification = Notification.find params[:id]
notification.mark_as_read
redirect_to notification.source
end
end
8 changes: 7 additions & 1 deletion app/models/challenge.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ class Challenge < ActiveRecord::Base

validates :name, :description, presence: true

after_create :post_notification

class << self
def in_chronological_order
order('created_at ASC')
Expand All @@ -16,4 +18,8 @@ def visible
def closed?
closes_at.past?
end
end

def post_notification
Notification.create_notifications_for self, to: User.all, title: "Новo предизвикателство: #{name}"
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Един празен ред на края на файла.

27 changes: 27 additions & 0 deletions app/models/notification.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
class Notification < ActiveRecord::Base
belongs_to :user
belongs_to :source, polymorphic: true

def mark_as_read
self.read = true
save!
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erghm, update_attributes! read: true?

end

class << self
def unread_for_user(user)
where(user_id: user.id, read: false)
end

def create_notifications_for(source, to: nil, title: nil)
unless to.nil?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return if to.nil?

Даже, return unless to.

Маха едно ниво на влагане.

to.find_each do |user|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not Zoidberg #each?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тука аз suggest-нах това prematurely, очаквайки, че може да се случат повечко събития. Това всъщност на дали ще е така, пък и повече неща отговарят на #each и може да се тества по-лесно.

Notification.create do |notification|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notification.create! може да помогнем, като изгърми мощно, ако променим схемата. create просто ще ни върне невалиден notification, който не е записан в базата.

notification.title = title
notification.source = source
notification.user = user
end
end
end
end
end
end
9 changes: 9 additions & 0 deletions app/models/reply.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
class Reply < Post
belongs_to :topic
belongs_to :user

attr_accessible :body

validates :topic_id, presence: true

after_create :post_notification

def topic_title
topic.title
end
Expand All @@ -13,4 +16,10 @@ def page_in_topic
replies_before_this = topic.replies.where('id < ?', id).count
replies_before_this / Reply.per_page + 1
end

private

def post_notification
Notification.create_notifications_for topic, to: topic.participants, title: "Нов отговор в тема: #{topic_title}"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Като го се замисля, това няма ли да създаде известие за човека, който е пуснал отговора?

end
end
6 changes: 6 additions & 0 deletions app/models/task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ class Task < ActiveRecord::Base
scope :checked, -> { where checked: true }
scope :in_chronological_order, -> { order 'created_at ASC' }

after_create :post_notification

class << self
def visible
in_chronological_order.where(hidden: false)
Expand Down Expand Up @@ -45,4 +47,8 @@ def restrictions_must_be_valid_yaml
rescue Psych::SyntaxError
errors.add :restrictions, :invalid
end

def post_notification
Notification.create_notifications_for self, to: User.all, title: "Нова задача: #{name}"
end
end
2 changes: 2 additions & 0 deletions app/models/topic.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
class Topic < Post
has_many :replies, -> { order 'created_at ASC' }

has_many :participants, -> { uniq }, through: :replies, source: :user
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вече тази релация ми стана сложна. Струва ми се, че ще е по-добра като метод.


attr_accessible :title, :body

Expand Down
1 change: 1 addition & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class User < ActiveRecord::Base
has_many :solutions
has_many :tips
has_many :attributions
has_many :notifications

attr_protected :full_name, :faculty_number, :email, :admin

Expand Down
1 change: 1 addition & 0 deletions app/views/layouts/application.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
%ul
- if user_signed_in?
%li Здрасти, #{link_to current_user.name, edit_profile_path}
%li.notifications-link #{link_to 'Известия', notifications_path} (#{@notifications.count})
%li= link_to 'Изход', destroy_user_session_path
- else
%li= link_to 'Вход', new_user_session_path
Expand Down
11 changes: 11 additions & 0 deletions app/views/notifications/index.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
%h1 Известия

%p
Общо известия:
%strong= @notifications.count

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

%table
%tbody
- @notifications.each do |notification|
%tr
%td.name.notification-name= link_to notification.title, notification
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

resources :vouchers, only: %w(index new create)

resources :notifications, only: %w(index show)

resources :announcements, except: %w(destroy)
resource :profile, only: %w(edit update)
resource :leaderboard, only: :show
Expand Down
19 changes: 19 additions & 0 deletions db/migrate/20150518105230_create_notifications.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class CreateNotifications < ActiveRecord::Migration
include ForeignKeys

def self.up
create_table(:notifications) do |t|
t.string :title, null: false
t.references :source, polymorphic: true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null: false?

t.boolean :read, default: false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null: false?

t.references :user
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null: false?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

t.timestamps
end

foreign_key :notifications, :user_id
end

def self.down
drop_table :notifications
end
end
45 changes: 45 additions & 0 deletions features/notifications.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# language: bg
Функционалност: Известия
Студентите получават известия, когато бъде публикувана задача,
предизвикателсвто или отговор във форум тема

Сценарий: Получаване на известие при публикуване на задача
Дадено че съм влязъл като администратор
Когато създам задача:
| Име | Първа задача |
| Условие | Напишете програма |
То трябва да съществува известие "Нова задача: Първа задача"

Сценарий: Получаване на известие при публикуване на предизвикателство
Дадено че съм влязъл като администратор
Когато създам предизвикателство "Четене на субтитри"
То трябва да съществува известие "Новo предизвикателство: Четене на субтитри"

Сценарий: Получаване на известие при публикуване на отговор в тема
Дадено че съм студент
Дадено че съществува тема "Въпрос"
И че темата "Въпрос" има "1" отговора, последния от които на "Петър Петров"
Когато отговоря на "Въпрос" с:
"""
Моят текст
"""
То трябва да съществува известие "Нов отговор в тема: Въпрос"

Сценарий: Потребителите виждат само непрочетени известия
Дадено че съм студент
Дадено че съществува известие "Прочетено известие"
Дадено че съществува непрочетено известие "Ново непрочетено известие"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дадено/И/И, не Дадено/Дадено/Дадено. Ditto за другите места.

Когато отворя страницата с известията
То трябва да виждам известие "Ново непрочетено известие"
И трябва да не виждам известие "Прочетено известие"
И броя на новите известия трябва да е 1

Сценарий: Когато избера ново известие съм пренасочен към страницата на първоизточника
Дадено че съм влязъл като администратор
Когато създам задача:
| Име | Първа задача |
| Условие | Напишете програма |
И отворя страницата с известията
И избера известието "Нова задача: Първа задача"
То трябва да съм на задачата "Първа задача"
И броя на новите известия трябва да е 0
39 changes: 39 additions & 0 deletions features/step_definitions/notifications_steps.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
Дадено 'че съществува известие "$title"' do |title|
create :notification, title: title, user_id: current_user.id, read: true
end

Дадено 'че съществува непрочетено известие "$title"' do |title|
create :notification, title: title, user_id: current_user.id, read: false
end

Когато 'отворя страницата с известията' do
visit notifications_path
end

Когато 'избера известието "$title"' do |title|
notification = Notification.find_by_title! title
visit notification_path(notification)
end

То 'трябва да виждам известие "$title"' do |title|
within ".notification-name" do
page.should have_content title
end
end

То 'трябва да не виждам известие "$title"' do |title|
within ".notification-name" do
page.should_not have_content title
end
end

То 'броя на новите известия трябва да е $count' do |count|
within ".notifications-link" do
page.should have_content "(#{count})"
end
end

То 'трябва да съществува известие "$title"' do |title|
notification = Notification.find_by_title! title
notification.should_not be_blank
end
22 changes: 22 additions & 0 deletions spec/controllers/notifications_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'spec_helper'

describe NotificationsController do
describe "GET index" do
it "assigns unread user notifications to @notifications" do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тоя тест е куц, защото контролена не прави това, което пише в името на теста.

controller.stub_chain(:current_user).and_return(:user)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Има хелпър за това някъде.

Notification.should_receive(:unread_for_user).with(:user).and_return('notifications')
get :index
assigns(:notifications).should eq 'notifications'
end
end

describe "GET show" do
notification = FactoryGirl.build_stubbed(:notification)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you even let bruh?


it "marks notification as read and redirects to its source" do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тук не тестваш, че notification-а се маркира като прочетен.

Отделно, това са два теста

it "marks the notification as read"
it "redirects to the notification's source"

Notification.should_receive(:find).with('3').and_return(notification)
get :show, id: '3'
response.should redirect_to notification.source
end
end
end
6 changes: 6 additions & 0 deletions spec/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@
user
end

factory :notification do
title 'Title'
association :source, factory: :topic
user
end

factory :reply do
body 'Body'
topic
Expand Down
9 changes: 9 additions & 0 deletions spec/models/notification_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
require 'spec_helper'

describe Notification do
it "can be marked as read" do
notification = create(:notification)
notification.mark_as_read
notification.read.should eq true
end
end
5 changes: 5 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,10 @@
config.include EmailSpec::Helpers, type: :mailer
config.include EmailSpec::Matchers, type: :mailer
config.include CustomPaths, type: :mailer
config.include Devise::TestHelpers, :type => :controller
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also nope.


config.before(:each, type: :controller) do
allow(Notification).to receive(:unread_for_user).and_return([])
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope.

end
end