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

New message notification #533

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

New message notification #533

wants to merge 12 commits into from

Conversation

stage-rl
Copy link
Collaborator

@stage-rl stage-rl commented Jan 5, 2025

No description provided.

@stage-rl stage-rl requested a review from jsuchal January 6, 2025 09:26
@stage-rl
Copy link
Collaborator Author

stage-rl commented Jan 6, 2025

@jsuchal nie je to zatial poriadne otestovane, kedze sa blbo testuje (potrebujem vyrobit na strane UPVS message na prevzatie). Fungovalo to na inom triggri, takze principialne by fungovat malo. Tak spravme tak, ze najprv kuknes koncept, ak zhruba OK, poprosim Luciu o zopar sprav, a otestujem a spravim nejake videjko z funkcnosti. Dikes

@@ -4,3 +4,8 @@
thread_messages: @thread_messages,
thread_last_message_draft_id: @thread_last_message_draft_id
) %>
<% if @notify %>
<%= turbo_frame_tag :new_messages_frame, target: "_top" do %>
<%= turbo_stream_from @message_thread %>
Copy link
Member

Choose a reason for hiding this comment

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

Security tu je ako riesene?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Security je postavene na predpokladoch (ktore som 100% neoveroval, ale verim, ze platia)

  1. Ked uz vidim vlakno, tak budem moct vidiet aj spravu, ktora pride po prevzati dorucenky
  2. Takyto subscription sa urobi len ked vidim dane vlakno (a subscription je zavesene na ID daneho vlakna), neviem ho vyrobit nijako odboku
  3. Jedine, co mi vysledok danej subscription da, je info o vzniku novej spravy a jej ID/linku na nu, co by nemalo nijako pomoct cloveku, co na tu spravu/vlakno nema pristup

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Podla dokumentacie je toto doriesene v tomto turbo_stream_from, resp. v Turbo::Streamable, ak spravne chapem nasledovne:

https://rubydoc.info/github/hotwired/turbo-rails/Turbo%2FStreamsHelper:turbo_stream_from

The stream name being generated is safe to embed in the HTML sent to a user without fear of tampering, as it is signed using Turbo.signed_stream_verifier

Copy link
Member

Choose a reason for hiding this comment

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

@stage-rl problem nie je tempering obsahu ale to, ze niekto sa napichne na stream a bude citat nieco co nema.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Skor si myslim, ze to maju vyriesene railsaci vnutri. Ale keby aj nie, max sa utocnik dozvie linku na vlakno, na ktore nema pravo.

Copy link
Member

Choose a reason for hiding this comment

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

Nemaju to doriesene (nemaju preco lebo turbo stream by default funguje ako public vec), treba doriesit autentifikaciu na urovni vytvarania channel/connection. Je to ale par riadkov https://binarysolo.blog/is-your-action-cable-connection-secure-when-using-turbo/

Copy link
Member

@jsuchal jsuchal left a comment

Choose a reason for hiding this comment

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

Treba to zovseobecnit.

@luciajanikova
Copy link
Member

@stage-rl s tymto sa ako dari, nepotrebujes nic od nas?

@stage-rl
Copy link
Collaborator Author

Tak cakal som nejaku reakciu na tie moje elaboraty, ale OK, nejako to spravim podla poziadavky a uvidime

@stage-rl stage-rl requested a review from jsuchal February 15, 2025 13:08
Gemfile Outdated
@@ -5,6 +5,7 @@ ruby '3.3.0'
gem 'rails', '~> 7.1'
gem 'rails-i18n'

gem 'actioncable-enhanced-postgresql-adapter'
Copy link
Member

Choose a reason for hiding this comment

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

Ak spravne pozeram, tak pod 8000 bytes sa to sprava rovnako ako default adapter, cize sem nemusime tahat zavislost na nejaky deravy mrtvy gem 2 roky stary. (partial co posielas mas < 2000 bytes)

@@ -4,3 +4,6 @@
thread_messages: @thread_messages,
thread_last_message_draft_id: @thread_last_message_draft_id
) %>
<%= turbo_frame_tag :new_messages_frame, target: "_top" do %>
<%= turbo_stream_from @message_thread %>
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

Zaujimave, ze ordering gemov tvoj linter riesi ale newline na konci nie. :)

</div>
<div class="ml-3">
<p class="text-sm font-medium text-blue-800">
<%= link_to message_thread_path(message.thread), class: "underline" do %>
Copy link
Member

Choose a reason for hiding this comment

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

tu nepotrebujes block tu ti staci link_to nazov, url, zvysok

Comment on lines 19 to 23
<button
type="button"
data-action="dismissible-alert#dismiss"
class="inline-flex rounded-md bg-blue-50 text-blue-500 hover:bg-blue-100 focus:ring-blue-600 focus:ring-offset-blue-50 p-1.5 focus:outline-none focus:ring-2 focus:ring-offset-2"
>
Copy link
Member

Choose a reason for hiding this comment

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

Tomuto formatovaniu som nikdy neprisiel na chut.


sign_in_as(:admin)
end
test 'should notify user on new message' do
Copy link
Member

Choose a reason for hiding this comment

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

tu dajme newline medzi testy

@@ -46,6 +46,9 @@ class Message < ApplicationRecord

after_update_commit ->(message) { EventBus.publish(:message_changed, message) }
after_destroy_commit ->(message) { EventBus.publish(:message_destroyed, message) }
after_create_commit do |message|
broadcast_render_later_to message.thread, partial: "messages/new_message_alert", locals: { message: message }
Copy link
Member

Choose a reason for hiding this comment

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

Do ktorej fronty sa toto zaradi? Beriem to tak, ze by to malo mat dost vysoku prioritu kedze to je userfacing.


test:
adapter: test
adapter: enhanced_postgresql

production:
Copy link
Member

Choose a reason for hiding this comment

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

V produkcii sa odporuca to bezat standalone, ale nemyslim si ze tu bude nejaky velky traffic ze by nas to malo trapit. https://guides.rubyonrails.org/action_cable_overview.html#running-standalone-cable-servers cc @celuchmarek co myslis?

Copy link
Member

@jsuchal jsuchal left a comment

Choose a reason for hiding this comment

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

Mas aj video/screen ako to vyzera?

@stage-rl
Copy link
Collaborator Author

failures_test_should_notify_user_on_new_message

@stage-rl stage-rl requested a review from jsuchal February 22, 2025 14:29
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.

3 participants