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

Note subscriptions db table #5284

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/controllers/api/notes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,8 @@ def add_comment(note, text, event, notify: true)
note.comments.map(&:author).uniq.each do |user|
UserMailer.note_comment_notification(comment, user).deliver_later if notify && user && user != current_user && user.visible?
end

NoteSubscription.find_or_create_by(:note => note, :user => current_user) if current_user
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simplified using the association?

Suggested change
NoteSubscription.find_or_create_by(:note => note, :user => current_user) if current_user
current_user&.note__subscriptions.find_or_create_by(:note => note)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would require one more &. after note__subscriptions, so it depends on whether you think that chained &.s are simplifications.

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe it does - if current_user is defined then the note_subscriptions association will always exist. It might evaluate to an empty list but the association object will exist and can have methods invoked on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Try running Rubocop.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm you're quite right, and rubocop is quite right, which I find quite odd but apparently ruby does continue evaluating the line after the first &. fails.

Personally I still think I prefer it but it's not a huge issue.

end
end
end
2 changes: 2 additions & 0 deletions app/models/note.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class Note < ApplicationRecord

has_many :comments, -> { left_joins(:author).where(:visible => true, :users => { :status => [nil, "active", "confirmed"] }).order(:created_at) }, :class_name => "NoteComment", :foreign_key => :note_id
has_many :all_comments, -> { left_joins(:author).order(:created_at) }, :class_name => "NoteComment", :foreign_key => :note_id, :inverse_of => :note
has_many :subscriptions, :class_name => "NoteSubscription"
has_many :subscribers, :through => :subscriptions, :source => :user

validates :id, :uniqueness => true, :presence => { :on => :update },
:numericality => { :on => :update, :only_integer => true }
Expand Down
20 changes: 20 additions & 0 deletions app/models/note_subscription.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# == Schema Information
#
# Table name: note_subscriptions
#
# user_id :bigint(8) not null, primary key
# note_id :bigint(8) not null, primary key
#
# Indexes
#
# index_note_subscriptions_on_note_id (note_id)
#
# Foreign Keys
#
# fk_rails_... (note_id => notes.id)
# fk_rails_... (user_id => users.id)
#
class NoteSubscription < ApplicationRecord
belongs_to :user
belongs_to :note
end
2 changes: 2 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ class User < ApplicationRecord
has_and_belongs_to_many :changeset_subscriptions, :class_name => "Changeset", :join_table => "changesets_subscribers", :foreign_key => "subscriber_id"
has_many :note_comments, :foreign_key => :author_id, :inverse_of => :author
has_many :notes, :through => :note_comments
has_many :note_subscriptions, :class_name => "NoteSubscription"
has_many :subscribed_notes, :through => :note_subscriptions, :source => :note

has_many :oauth2_applications, :class_name => Doorkeeper.config.application_model.name, :as => :owner
has_many :access_grants, :class_name => Doorkeeper.config.access_grant_model.name, :foreign_key => :resource_owner_id
Expand Down
8 changes: 8 additions & 0 deletions db/migrate/20241022141247_create_note_subscriptions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class CreateNoteSubscriptions < ActiveRecord::Migration[7.2]
def change
create_table :note_subscriptions, :primary_key => [:user_id, :note_id] do |t|
t.references :user, :foreign_key => true, :index => false
t.references :note, :foreign_key => true
end
end
end
42 changes: 42 additions & 0 deletions db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,16 @@ CREATE SEQUENCE public.note_comments_id_seq
ALTER SEQUENCE public.note_comments_id_seq OWNED BY public.note_comments.id;


--
-- Name: note_subscriptions; Type: TABLE; Schema: public; Owner: -
--

CREATE TABLE public.note_subscriptions (
user_id bigint NOT NULL,
note_id bigint NOT NULL
);


--
-- Name: notes; Type: TABLE; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2010,6 +2020,14 @@ ALTER TABLE ONLY public.note_comments
ADD CONSTRAINT note_comments_pkey PRIMARY KEY (id);


--
-- Name: note_subscriptions note_subscriptions_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.note_subscriptions
ADD CONSTRAINT note_subscriptions_pkey PRIMARY KEY (user_id, note_id);


--
-- Name: notes notes_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2498,6 +2516,13 @@ CREATE INDEX index_note_comments_on_body ON public.note_comments USING gin (to_t
CREATE INDEX index_note_comments_on_created_at ON public.note_comments USING btree (created_at);


--
-- Name: index_note_subscriptions_on_note_id; Type: INDEX; Schema: public; Owner: -
--

CREATE INDEX index_note_subscriptions_on_note_id ON public.note_subscriptions USING btree (note_id);


--
-- Name: index_oauth_access_grants_on_application_id; Type: INDEX; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2953,6 +2978,14 @@ ALTER TABLE ONLY public.diary_entry_subscriptions
ADD CONSTRAINT diary_entry_subscriptions_user_id_fkey FOREIGN KEY (user_id) REFERENCES public.users(id);


--
-- Name: note_subscriptions fk_rails_2c1913f293; Type: FK CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.note_subscriptions
ADD CONSTRAINT fk_rails_2c1913f293 FOREIGN KEY (note_id) REFERENCES public.notes(id);


--
-- Name: oauth_access_grants fk_rails_330c32d8d9; Type: FK CONSTRAINT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2993,6 +3026,14 @@ ALTER TABLE ONLY public.active_storage_variant_records
ADD CONSTRAINT fk_rails_993965df05 FOREIGN KEY (blob_id) REFERENCES public.active_storage_blobs(id);


--
-- Name: note_subscriptions fk_rails_a352f4eced; Type: FK CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.note_subscriptions
ADD CONSTRAINT fk_rails_a352f4eced FOREIGN KEY (user_id) REFERENCES public.users(id);


--
-- Name: oauth_access_grants fk_rails_b4b53e07b8; Type: FK CONSTRAINT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -3356,6 +3397,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('23'),
('22'),
('21'),
('20241022141247'),
('20240913171951'),
('20240912181413'),
('20240910175616'),
Expand Down
115 changes: 104 additions & 11 deletions test/controllers/api/notes_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,12 @@ def test_routes
)
end

def test_create_success
def test_create_anonymous_success
assert_difference "Note.count", 1 do
assert_difference "NoteComment.count", 1 do
post api_notes_path(:lat => -1.0, :lon => -1.0, :text => "This is a comment", :format => "json")
assert_no_difference "NoteSubscription.count" do
post api_notes_path(:lat => -1.0, :lon => -1.0, :text => "This is a comment", :format => "json")
end
end
end
assert_response :success
Expand Down Expand Up @@ -135,7 +137,7 @@ def test_create_success
assert_nil js["properties"]["comments"].last["user"]
end

def test_create_fail
def test_create_anonymous_fail
assert_no_difference "Note.count" do
assert_no_difference "NoteComment.count" do
post api_notes_path(:lon => -1.0, :text => "This is a comment")
Expand Down Expand Up @@ -200,14 +202,44 @@ def test_create_fail
assert_response :bad_request
end

def test_create_success
user = create(:user)
auth_header = bearer_authorization_header user
assert_difference "Note.count", 1 do
assert_difference "NoteComment.count", 1 do
assert_difference "NoteSubscription.count", 1 do
post api_notes_path(:lat => -1.0, :lon => -1.0, :text => "This is a comment", :format => "json"), :headers => auth_header
end
end
end
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
assert_not_nil js
assert_equal "Feature", js["type"]
assert_equal "Point", js["geometry"]["type"]
assert_equal [-1.0, -1.0], js["geometry"]["coordinates"]
assert_equal "open", js["properties"]["status"]
assert_equal 1, js["properties"]["comments"].count
assert_equal "opened", js["properties"]["comments"].last["action"]
assert_equal "This is a comment", js["properties"]["comments"].last["text"]
assert_equal user.display_name, js["properties"]["comments"].last["user"]

note = Note.last
subscription = NoteSubscription.last
Copy link
Member

Choose a reason for hiding this comment

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

This (and all the other similar versions in other tests) is relying on nothing else having created notes or users in a way that overlaps this test and I'm not sure how safe that is when tests are running in parallel?

Maybe it would be better to get the node ID from the JSON response and then do assert Note.exists?(id) and a similar exists assertion on the subscription record?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how safe that is when tests are running in parallel?

Parallel tests are run in separate databases (e.g. openstreetmap_test-0 etc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ... is relying on nothing else having created notes

There is a check assert_difference "Note.count", 1

(and all the other similar versions in other tests)

Other tests in this file or other tests elsewhere? You can find similar uses of .last from years ago.

assert_equal user, subscription.user
assert_equal note, subscription.note
end

def test_comment_success
open_note_with_comment = create(:note_with_comments)
user = create(:user)
auth_header = bearer_authorization_header user
assert_difference "NoteComment.count", 1 do
assert_no_difference "ActionMailer::Base.deliveries.size" do
perform_enqueued_jobs do
post comment_api_note_path(open_note_with_comment, :text => "This is an additional comment", :format => "json"), :headers => auth_header
assert_difference "NoteSubscription.count", 1 do
assert_no_difference "ActionMailer::Base.deliveries.size" do
perform_enqueued_jobs do
post comment_api_note_path(open_note_with_comment, :text => "This is an additional comment", :format => "json"), :headers => auth_header
end
end
end
end
Expand All @@ -222,6 +254,10 @@ def test_comment_success
assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
assert_equal user.display_name, js["properties"]["comments"].last["user"]

subscription = NoteSubscription.last
assert_equal user, subscription.user
assert_equal open_note_with_comment, subscription.note

get api_note_path(open_note_with_comment, :format => "json")
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
Expand All @@ -233,7 +269,9 @@ def test_comment_success
assert_equal "commented", js["properties"]["comments"].last["action"]
assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
assert_equal user.display_name, js["properties"]["comments"].last["user"]
end

def test_comment_with_notifications_success
# Ensure that emails are sent to users
first_user = create(:user)
second_user = create(:user)
Expand All @@ -247,9 +285,11 @@ def test_comment_success
auth_header = bearer_authorization_header third_user

assert_difference "NoteComment.count", 1 do
assert_difference "ActionMailer::Base.deliveries.size", 2 do
perform_enqueued_jobs do
post comment_api_note_path(note_with_comments_by_users, :text => "This is an additional comment", :format => "json"), :headers => auth_header
assert_difference "NoteSubscription.count", 1 do
assert_difference "ActionMailer::Base.deliveries.size", 2 do
perform_enqueued_jobs do
post comment_api_note_path(note_with_comments_by_users, :text => "This is an additional comment", :format => "json"), :headers => auth_header
end
end
end
end
Expand All @@ -264,6 +304,10 @@ def test_comment_success
assert_equal "This is an additional comment", js["properties"]["comments"].last["text"]
assert_equal third_user.display_name, js["properties"]["comments"].last["user"]

subscription = NoteSubscription.last
assert_equal third_user, subscription.user
assert_equal note_with_comments_by_users, subscription.note

email = ActionMailer::Base.deliveries.find { |e| e.to.first == first_user.email }
assert_not_nil email
assert_equal 1, email.to.length
Expand All @@ -290,6 +334,43 @@ def test_comment_success
ActionMailer::Base.deliveries.clear
end

def test_comment_twice_success
open_note_with_comment = create(:note_with_comments)
user = create(:user)
auth_header = bearer_authorization_header user
assert_difference "NoteComment.count", 1 do
assert_difference "NoteSubscription.count", 1 do
assert_no_difference "ActionMailer::Base.deliveries.size" do
perform_enqueued_jobs do
post comment_api_note_path(open_note_with_comment, :text => "This is an additional comment", :format => "json"), :headers => auth_header
end
end
end
end
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
assert_not_nil js
assert_equal 2, js["properties"]["comments"].count

subscription = NoteSubscription.last
assert_equal user, subscription.user
assert_equal open_note_with_comment, subscription.note

assert_difference "NoteComment.count", 1 do
assert_no_difference "NoteSubscription.count" do
assert_no_difference "ActionMailer::Base.deliveries.size" do
perform_enqueued_jobs do
post comment_api_note_path(open_note_with_comment, :text => "This is a second additional comment", :format => "json"), :headers => auth_header
end
end
end
end
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
assert_not_nil js
assert_equal 3, js["properties"]["comments"].count
end

def test_comment_fail
open_note_with_comment = create(:note_with_comments)

Expand Down Expand Up @@ -346,7 +427,9 @@ def test_close_success

auth_header = bearer_authorization_header user

post close_api_note_path(open_note_with_comment, :text => "This is a close comment", :format => "json"), :headers => auth_header
assert_difference "NoteSubscription.count", 1 do
post close_api_note_path(open_note_with_comment, :text => "This is a close comment", :format => "json"), :headers => auth_header
end
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
assert_not_nil js
Expand All @@ -358,6 +441,10 @@ def test_close_success
assert_equal "This is a close comment", js["properties"]["comments"].last["text"]
assert_equal user.display_name, js["properties"]["comments"].last["user"]

subscription = NoteSubscription.last
assert_equal user, subscription.user
assert_equal open_note_with_comment, subscription.note

get api_note_path(open_note_with_comment.id, :format => "json")
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
Expand Down Expand Up @@ -400,7 +487,9 @@ def test_reopen_success

auth_header = bearer_authorization_header user

post reopen_api_note_path(closed_note_with_comment, :text => "This is a reopen comment", :format => "json"), :headers => auth_header
assert_difference "NoteSubscription.count", 1 do
post reopen_api_note_path(closed_note_with_comment, :text => "This is a reopen comment", :format => "json"), :headers => auth_header
end
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
assert_not_nil js
Expand All @@ -412,6 +501,10 @@ def test_reopen_success
assert_equal "This is a reopen comment", js["properties"]["comments"].last["text"]
assert_equal user.display_name, js["properties"]["comments"].last["user"]

subscription = NoteSubscription.last
assert_equal user, subscription.user
assert_equal closed_note_with_comment, subscription.note

get api_note_path(closed_note_with_comment, :format => "json")
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
Expand Down