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

notification #607

Merged
merged 3 commits into from Jun 28, 2023
Merged

notification #607

merged 3 commits into from Jun 28, 2023

Conversation

usernaimandrey
Copy link
Contributor

Поправил работу уведомлений
Снимок экрана от 2023-06-15 15-34-37
Снимок экрана от 2023-06-15 15-34-25

Comment on lines +9 to +10
@career_member = Career::MemberMutator.create(resource_career, career_member_params)
if @career_member.persisted?
Copy link
Contributor

Choose a reason for hiding this comment

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

а это как связано с уведомлениями?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Уведомление с kind: new_career_member создается когда создается новый career_member то есть назначается карерный трек

td = notification.aasm(:state).human_state
td = l notification.created_at, format: :short
td
.text-center
Copy link
Contributor

Choose a reason for hiding this comment

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

в соседнем ПРе решили что текст центр это не очень для кнопок

Copy link
Contributor

@amshkv amshkv left a comment

Choose a reason for hiding this comment

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

не хватает БОЛЬШОЙ ОГРОМНОЙ КНОПКНИ ОТМЕТИТЬ ВСЕ УВЕДОМЛЕНИЯ КАК ПРОЧИТАННЫЕ

Copy link
Contributor

@corsicanec82 corsicanec82 left a comment

Choose a reason for hiding this comment

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

Вроде выглядит всё ок.
Для приличия оставил небольшой коммент по именованию )

@@ -7,5 +7,9 @@ def full_name
"#{first_name} #{last_name}"
end

def unread_notifications?
Copy link
Contributor

Choose a reason for hiding this comment

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

я бы в название добавил has
has_unread_nifications?

Copy link
Contributor

Choose a reason for hiding this comment

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

рубокоп будет ругаться - типа в рельсе так не принято методы делать has_ и is_. так что тут норм

Copy link
Contributor

Choose a reason for hiding this comment

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

Про is знаю что не юзаем, а вот про has не знал.
Смотрел по аналогии как у нас сделано, а там мы используем has_ в названиях.
Например:
https://github.com/Hexlet/hexlet/blob/ab6a7cae6338b48a257c0bcbf88869020d85be8f/app/controllers/concerns/redirect_back_concern.rb#L8
https://github.com/Hexlet/hexlet/blob/ab6a7cae6338b48a257c0bcbf88869020d85be8f/app/models/lesson.rb#L254
и много где есть в принципе

Copy link
Contributor

Choose a reason for hiding this comment

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

ага ты прав, чет мне приведлось значит

@@ -7,5 +7,9 @@ def full_name
"#{first_name} #{last_name}"
end

def unread_notifications?
notifications.unread.any?
Copy link
Contributor

Choose a reason for hiding this comment

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

но мы обычно не пишем такие методы а добавляем связь
has_many :unread_notifications, -> { unread }, class_name: 'Notification', inverse_of..., dependent...

и потом где нужно просто его дергаем current_user.unread_notifications.any?

@usernaimandrey usernaimandrey merged commit d275a66 into main Jun 28, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants