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

Report feature #953

Closed
wants to merge 0 commits into
base: master
from

Conversation

@aSquare14
Member

aSquare14 commented Sep 2, 2018

Description

  • Admin column added to the User table. User can been made an admin via the rails console by maintainers.
  • Report model includes
    • reporter_id
    • reportee_id
    • reasons
    • comment_id (if applicable)
  • 'Report' button displayed for
    • Each ally in the allies list.
    • User profiles
    • Comments in Moments, Strategies and Meetings.
  • Report dashboard can be viewed by the admin. Admin can delete the reports, mail the users.

Corresponding Issues

Worked on the following tickets related to Issue #14:

  • [Create admin User privileges - #901 ]
  • [Report Model - #863 ]
  • [Report button on User profiles - #870 ]
  • [Report Dashboard for admin - #861 ]
  • [Admin Dashboard UI - #948 ]
  • [Ban Users - #954 ]

Test Coverage

In Progress

@camillevilla

camillevilla requested changes Sep 5, 2018 edited

Hi @aSquare14 and @prateksha! Great clean up and consolidation of the PR. I think a good next step to focus on is making the ActiveRecord associations between User and Report.

The first step will be changing reporter_id and reportee_idto integers in your migrations and report factory. Next, focus on associations. I've discussed examples from this Odin Project blog post in the comments below.

Ideally we should be able to open up the interactive console (`bin rails console) and do the following:

  • Create a new report from our factory with FactoryBot.create(:report)
  • Inspect that new Report with Report.find(1) and see that it has integers for reporter_id and reportee_id
  • Get additional details about the reportee through our Report (which user submitted this report?). Report.find(1).reportee_id should return a User record #<User id: 1, email: "test1@example.com", …)
  • Let's also answer the opposite question: How many reports has this user submitted? User.find(1).submitted_reports should return an array of Reports info, like #<Report:0x007fea65c840b8 id: 1, reporter_id: 1, reportee_id: 2, …
Show resolved Hide resolved spec/factories.rb Outdated
Show resolved Hide resolved db/migrate/20180831162612_create_reports.rb Outdated
Show resolved Hide resolved app/models/report.rb Outdated
Show resolved Hide resolved app/models/report.rb Outdated
Show resolved Hide resolved app/models/user.rb Outdated

@prateksha prateksha force-pushed the prateksha:ReportFeature branch from 1e6179b to 6f0af7b Sep 7, 2018

@aSquare14 aSquare14 force-pushed the prateksha:ReportFeature branch 4 times, most recently from a8af72c to 0d8dcd6 Sep 7, 2018

@julianguyen

Great work so far! Excellent progress :)
Most of my feedback is stylistic, let me know if you have any questions!

@@ -0,0 +1,2 @@
// Place all the behaviors and hooks related to the matching controller here.

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

I would delete this file since there's nothing in it.

@@ -0,0 +1,3 @@
// Place all the styles related to the Reports controller here.

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

Similar comment above here

@@ -333,6 +343,10 @@ def comment_for(model_name)
respond_not_saved
end
def comment_reportable?(data, _data_type)

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

Is _data_type necessary since it's not being used?

@@ -333,6 +343,10 @@ def comment_for(model_name)
respond_not_saved
end
def comment_reportable?(data, _data_type)
data.comment_by != current_user.id

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

Perhaps just pass in comment_by directly instead of the whole data object

@@ -0,0 +1,3 @@
# frozen_string_literal: true
module ReportsHelper

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

Let's remove this since it's not used

<div class="container">
<h1>Reports Table for Admin </h1>
<table class="table table-hover" border=1px >

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

This is fine for now, but ideally, we should be using flex box for tables. We can migrate that in another PR.

@@ -0,0 +1 @@
<% title "#{'You have Reported ! Reach out to us for help.'}" %>

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

This text should be in i18n

<div class="table">
<div class="field">
<div class="label">
<%= f.label 'Why are you reporting ?' %>

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

This text should be in i18n

<%= text_area(:report, :reasons) %>
</div>
<div>
<%= f.submit 'Submit' %>

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

We have an i18n key for this already

@@ -64,6 +64,7 @@ en:
add_all: 'Add all'
select_all: 'Select all'
remove: 'Remove'
report: 'Report'

This comment has been minimized.

@julianguyen

julianguyen Sep 9, 2018

Member

Remember that every added key in en.yml needs to be translated into other languages. Feel free to use Google Translate for now.

@prateksha prateksha force-pushed the prateksha:ReportFeature branch from 9cbe776 to f89aea2 Sep 21, 2018

@julianguyen

Yay paired with Prateksha on resolving merge conflicts 💃

Went through the PR again and found some things to fix, let me know if there are any questions.

Please also add a feature/acceptance test to test out the paths correctly.

E.g. Test out viewing a profile or comment, hitting report, and filling out the report.

validates :reporter_id, presence: true
validates :reasons, presence: true
after_create :send_mail_reports
#make sure reporter_id and reportee_id are different

This comment has been minimized.

@julianguyen

julianguyen Sep 21, 2018

Member

Are we missing logic here to do this?

@@ -0,0 +1,15 @@
<% title "#{'New Report'}" %>
<%= form_for(:search, :url => {:action => "create", :ally_id => params[:ally_id], :comment_id => params[:comment_id]}, :method => "post") do |f| %>
<div class="table">

This comment has been minimized.

@julianguyen

julianguyen Sep 21, 2018

Member

You can get rid of the table class.

@@ -0,0 +1,80 @@
<div id="header">

This comment has been minimized.

@julianguyen

julianguyen Sep 21, 2018

Member

I actually deleted this file, you'll want to put the link to the Admin Dashboard in /app/views/shared/_dashboard_nav_links.html.erb

@alvarocasadoc

Nice work! 👏 I've let some comments that I would like to discuss with you about the user_id field in the Report model

@@ -0,0 +1,5 @@
class AddUseridToReports < ActiveRecord::Migration[5.0]
def change
add_column :reports, :user_id, :integer

This comment has been minimized.

@alvarocasadoc

alvarocasadoc Sep 21, 2018

Collaborator

In case you decide to update the models relationship, this migration could be removed.

# created_at :datetime not null
# updated_at :datetime not null
# comment_id :integer
# user_id :integer

This comment has been minimized.

@alvarocasadoc

alvarocasadoc Sep 21, 2018

Collaborator

I have a suggestion for you here. I see that you are currently storing both the reporter and the reportee user ids, but also there's this extra user_id field. I believe this is not needed to store the "owner" of this report, because you can use the reporter_id for that purpose. In that case, you should also update the relationship in the model.

#
class Report < ApplicationRecord
belongs_to :user

This comment has been minimized.

@alvarocasadoc

alvarocasadoc Sep 21, 2018

Collaborator

In that case, you could update the models relationship by specifying the foreign key that points to the users table. In this case, reporter_id:
belongs_to :user, class_name: 'User', foreign_key: 'reporter_id'

@julianguyen

This comment has been minimized.

Member

julianguyen commented Sep 28, 2018

I'm happy to pair on resolving the merge conflicts whenever you two have time :)

@aSquare14 aSquare14 closed this Sep 30, 2018

@aSquare14 aSquare14 force-pushed the prateksha:ReportFeature branch from ef3241c to 426c92e Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment