Deprecate answers model #11308

anirudhprabhakaran3 · 2022-07-26T17:23:33Z

Fixes #956

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

gitpod-io · 2022-07-26T17:23:42Z

anirudhprabhakaran3 · 2022-07-26T17:26:17Z

app/controllers/questions_controller.rb

@@ -90,8 +90,7 @@ def recently_commented
      .where(status: 1)
    @pagy, @questions = pagy(filter_questions_by_tag(@questions, params[:tagnames])
      .joins(:comments)
-      .order('comments.timestamp DESC')
-      .group('node.nid'), items: 24)


I have removed the group function from here for two reasons:

The @questions we are filtering on are already being grouped by node.nid.

The MySQL issue that is coming up was that ordering was happening on a variable on which grouping is not happening.

I believe that because of this, the behaviour of the code should not change, and we should get the same results.

I think we are doing this to order it by the comment timestamps, but because nodes can have multiple comments, we had been seeing them appear more than once, so we re-added the group to ensure each node appears only once. But if you can confirm for sure that the duplication isn't happening, I like a simpler query!

Hmm...on the data that I have I'm unable to see any duplication, but I think it is safer to keep it in that way then. I'll revert this change, thanks!

codecov · 2022-07-26T17:28:12Z

Codecov Report

Merging #11308 (2f6e6a6) into main (3499c8a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #11308      +/-   ##
==========================================
+ Coverage   82.33%   82.46%   +0.12%     
==========================================
  Files          98       97       -1     
  Lines        5996     5972      -24     
==========================================
- Hits         4937     4925      -12     
+ Misses       1059     1047      -12

Impacted Files	Coverage Δ
app/api/srch/search.rb	`70.51% <ø> (+3.84%)`	⬆️
app/models/node.rb	`91.10% <ø> (-0.02%)`	⬇️
app/controllers/application_controller.rb	`90.62% <100.00%> (ø)`
app/controllers/home_controller.rb	`95.77% <100.00%> (-0.18%)`	⬇️
app/models/comment.rb	`77.70% <100.00%> (-0.08%)`	⬇️
app/models/user.rb	`86.49% <100.00%> (-0.15%)`	⬇️
app/controllers/comment_controller.rb	`83.07% <0.00%> (+0.26%)`	⬆️
app/controllers/map_controller.rb	`50.33% <0.00%> (+0.33%)`	⬆️
app/services/execute_search.rb	`94.44% <0.00%> (+5.55%)`	⬆️

anirudhprabhakaran3 · 2022-07-26T17:29:04Z

Also commenting so that there is a track on the planning issue.

#11185

anirudhprabhakaran3 · 2022-07-26T17:29:50Z

Hmm, I notice (nearly) all of the tests have failed. Interesting. Let me check what precisely the logs are,

anirudhprabhakaran3 · 2022-07-26T17:30:53Z

Looks like it is a CodeCov issue?

x> No token specified or token is empty

@jywarren could you help me out here, please? Thanks!

jywarren · 2022-08-01T00:04:04Z

So strange. But CodeCove does get flaky sometimes. I'll try restarting it?

jywarren · 2022-08-01T00:04:28Z

Ah wait, i now see this error:

Minitest::UnexpectedError:         ActiveRecord::StatementInvalid: Mysql2::Error: Table 'plots.answers' doesn't exist: SHOW FULL FIELDS FROM `answers`

jywarren · 2022-08-01T00:06:18Z

I think we need to continue searching for where we depend on answers code! I mean, there are a lot: https://github.com/publiclab/plots2/search?q=answers

jywarren · 2022-08-01T00:06:46Z

If you're not sure what to do in one of these places, feel free to leave a comment at the end of the line, and I can make a code suggestion inline in this PR!

app/controllers/home_controller.rb

test/functional/api/search_api_test.rb

test/functional/home_controller_test.rb

test/functional/wiki_controller_test.rb

test/unit/doc_result_test.rb

app/controllers/home_controller.rb

jywarren · 2022-08-29T15:51:32Z

app/controllers/questions_controller.rb

@@ -90,8 +90,7 @@ def recently_commented
      .where(status: 1)
    @pagy, @questions = pagy(filter_questions_by_tag(@questions, params[:tagnames])
      .joins(:comments)
-      .order('comments.timestamp DESC')
-      .group('node.nid'), items: 24)


I think we are doing this to order it by the comment timestamps, but because nodes can have multiple comments, we had been seeing them appear more than once, so we re-added the group to ensure each node appears only once. But if you can confirm for sure that the duplication isn't happening, I like a simpler query!

jywarren · 2022-08-29T15:54:12Z

app/views/home/home.html.erb

@@ -126,6 +125,6 @@
 </style>

 <% cache('feature_home-footer', skip_digest: true) do %>
-  <%= feature("home-footer") %>


Hi, was this done incorrectly somehow? It might just be a feature we're not currently using. I think footer-notice is the topics table, so it might display twice if we add it here? But I could be wrong.

Can you confirm this? Then I think we are ready to merge, thanks!

Hey! So in the older home.html.erb, this is there so I haven't changed it yet. Would you like me to remove this?

Reference:

plots2/app/views/home/home.html.erb

Lines 128 to 130 in 5aa8720

<% cache('feature_home-footer', skip_digest: true) do %>

<%= feature("home-footer") %>

<% end %>

jywarren · 2022-08-29T15:54:19Z

db/migrate/20220726161355_drop_answers.rb

@@ -0,0 +1,5 @@
+class DropAnswers < ActiveRecord::Migration[5.2]
+  def change
+    drop_table :answers


test/functional/api/search_api_test.rb

test/unit/doc_result_test.rb

test/functional/home_controller_test.rb

github-actions · 2022-09-03T19:16:36Z

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8121896650/artifacts/351564547

codeclimate · 2022-09-03T19:21:02Z

Code Climate has analyzed commit 2f6e6a6 and detected 0 issues on this pull request.

View more on Code Climate.

github-actions · 2022-09-03T19:30:14Z

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8121959924/artifacts/351569761

anirudhprabhakaran3 · 2022-09-03T19:31:06Z

All tests passing!! 🥳 🎉

jywarren · 2022-09-05T01:05:59Z

test/functional/home_controller_test.rb

@@ -11,7 +11,7 @@ def setup

    get :home
    assert_response :success
-    assert_select "title", Sanitize.clean('&#127880;') + (" Public Lab: #{title}")
+    assert_select "title", Sanitize.clean('&#127880;') + (" Public Lab: #{title}") # Test failing here


Oops just need to remove this comment!

jywarren · 2022-09-05T01:06:09Z

test/functional/wiki_controller_test.rb

@@ -484,7 +484,7 @@ def teardown

    assert_response :success
    assert_template :index
-    assert_select "title", Sanitize.clean('&#127880;') + (" Public Lab: Popular wiki pages")
+    assert_select "title", Sanitize.clean('&#127880;') + (" Public Lab: Popular wiki pages") # Test failing here


jywarren

all good except 2 comments! Great work!

jywarren · 2022-09-05T14:02:25Z

app/views/home/home.html.erb

@@ -1,7 +1,6 @@
 <% cache('feature_home-intro', skip_digest: true) do %>
  <%= feature("home-intro") %>
 <% end %>
-


Ah i see in the final diff nothing has changed. Thank you, just checking! Merging!

jywarren · 2022-09-05T14:02:54Z

Great work, a big PR!!!

Deprecate answers model

c80faa1

anirudhprabhakaran3 requested review from a team as code owners July 26, 2022 17:23

anirudhprabhakaran3 commented Jul 26, 2022

View reviewed changes

anirudhprabhakaran3 added 3 commits August 9, 2022 09:39

Merge branch 'main' into delete_answers_model

8a735e1

Fixed unit and functional tests

a13079c

Comments on parts that need clarification

0538ae1

anirudhprabhakaran3 commented Aug 27, 2022

View reviewed changes

jywarren reviewed Aug 29, 2022

View reviewed changes

anirudhprabhakaran3 added 2 commits September 3, 2022 15:11

Fixed the activity function in home controller

3778cb5

More fixes to functional tests

414dc03

Fixed a unit test

2f6e6a6

jywarren reviewed Sep 5, 2022

View reviewed changes

jywarren approved these changes Sep 5, 2022

View reviewed changes

jywarren merged commit ef4a372 into publiclab:main Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate answers model #11308

Deprecate answers model #11308

anirudhprabhakaran3 commented Jul 26, 2022 •

edited

Loading

gitpod-io bot commented Jul 26, 2022

anirudhprabhakaran3 Jul 26, 2022

jywarren Aug 29, 2022

anirudhprabhakaran3 Aug 29, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

anirudhprabhakaran3 commented Jul 26, 2022

anirudhprabhakaran3 commented Jul 26, 2022

anirudhprabhakaran3 commented Jul 26, 2022

jywarren commented Aug 1, 2022

jywarren commented Aug 1, 2022

jywarren commented Aug 1, 2022

jywarren commented Aug 1, 2022

jywarren Aug 29, 2022

jywarren Aug 29, 2022

jywarren Sep 3, 2022

anirudhprabhakaran3 Sep 4, 2022

jywarren Aug 29, 2022

github-actions bot commented Sep 3, 2022

codeclimate bot commented Sep 3, 2022

github-actions bot commented Sep 3, 2022

anirudhprabhakaran3 commented Sep 3, 2022

jywarren Sep 5, 2022

jywarren Sep 5, 2022

jywarren left a comment

jywarren Sep 5, 2022

jywarren commented Sep 5, 2022

	<% cache('feature_home-footer', skip_digest: true) do %>
	<%= feature("home-footer") %>
	<% end %>

Deprecate answers model #11308

Deprecate answers model #11308

Conversation

anirudhprabhakaran3 commented Jul 26, 2022 • edited Loading

gitpod-io bot commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudhprabhakaran3 Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

anirudhprabhakaran3 commented Jul 26, 2022

anirudhprabhakaran3 commented Jul 26, 2022

anirudhprabhakaran3 commented Jul 26, 2022

jywarren commented Aug 1, 2022

jywarren commented Aug 1, 2022

jywarren commented Aug 1, 2022

jywarren commented Aug 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2022

codeclimate bot commented Sep 3, 2022

github-actions bot commented Sep 3, 2022

anirudhprabhakaran3 commented Sep 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Sep 5, 2022

anirudhprabhakaran3 commented Jul 26, 2022 •

edited

Loading

anirudhprabhakaran3 Aug 29, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading