FEATURE: Simplify plugin and rely on new page_visited event from core #32
Conversation
|
|
||
| visit_record = DiscoursePageVisits::PageVisit.last | ||
| expect(visit_record.ip_address).to eq("192.168.1.100") | ||
| end |
There was a problem hiding this comment.
Actually, another thought. Are we duplicating tests here?
Core is testing and ensuring that events are sent. Shall the plugin just ensure that events are properly handled?
So in that case, instead of sending HTTP requests, we can mock DiscourseEvents and ensure that the plugin is reacting correctly by creating the correct PageVisit records.
I don't have a strong opinion here and if you think that overarching integration test is safer, please keep it.
There was a problem hiding this comment.
I think ideally we'd have a system spec where in one session the user visits the site, and then in another session of an admin user, we go to the page where we displayed page views numbers and assert that the number increased. But we don't currently have such page, so the next best thing is an integration spec.
There was a problem hiding this comment.
Core is testing and ensuring that events are sent. Shall the plugin just ensure that events are properly handled?
So in that case, instead of sending HTTP requests, we can mock DiscourseEvents and ensure that the plugin is reacting correctly by creating the correct PageVisit records.
I agree with @KrisKotlarek here that we are duplicating what core is doing. From the plugin perspective, it shouldn't need to be concerned about the various request tracking headers that core implements. Just based on the code that we wrote for this plugin, we only need to test that the page_visit event is correctly handled when triggered by core.
| add_column :page_visits, :referer, :string, limit: 2000 | ||
| add_column :page_visits, :session_id, :string, limit: 32, null: false |
There was a problem hiding this comment.
Since the page_visits table isn't really use anywhere in the interface? What do you think about creating a new table instead so that old data is not mixed with the new data. We also don't have to carry legacy columns around.
Draft PR until discourse/discourse#36899 is finalized/merged.
cc @KrisKotlarek