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

Al/appeals 45664 #23279

Open
wants to merge 21 commits into
base: feature/APPEALS-45267
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 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
68 changes: 68 additions & 0 deletions app/controllers/saved_searches_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# frozen_string_literal: true

class SavedSearchesController < ApplicationController
include ValidationConcern
almorbah marked this conversation as resolved.
Show resolved Hide resolved

before_action :react_routed, :verify_vha_admin

PERMITTED_PARAMS = [
:name,
:description,
saved_search: {}
].freeze

def index
searches = SavedSearch.all_saved_searches
almorbah marked this conversation as resolved.
Show resolved Hide resolved
my_search = SavedSearch.my_saved_searches(current_user)
respond_to do |format|
format.html { render "index" }
format.json do
render json:
{ all_searches: SavedSearchSerializer.new(searches).serializable_hash[:data],
user_searches: SavedSearchSerializer.new(my_search).serializable_hash[:data] }
end
end
end

def show
search = SavedSearch.find_by_id(params[:id])
almorbah marked this conversation as resolved.
Show resolved Hide resolved
respond_to do |format|
format.html { render "show" }
format.json { render json: SavedSearchSerializer.new(search).serializable_hash[:data] }
end
end

def create
@search = current_user.saved_searches.new(save_search_create_params)

return render json: { message: "Search has been successfully created" }, status: :created if @search.save

render json: { message: "Error creating save search" }, status: :unprocessable_entity
end

def destroy
@search = current_user.saved_searches.find(params[:id])
almorbah marked this conversation as resolved.
Show resolved Hide resolved
@search.destroy!
render json: { status: :ok }
end

private

def save_search_create_params
params.require(:search).permit(PERMITTED_PARAMS)
end

def verify_vha_admin
almorbah marked this conversation as resolved.
Show resolved Hide resolved
return true if current_user.vha_business_line_admin_user?

redirect_to_unauthorized
end

def redirect_to_unauthorized
Rails.logger.info("User with roles #{current_user.roles.join(', ')} "\
"couldn't access #{request.original_url}")

session["return_to"] = request.original_url
redirect_to "/unauthorized"
end
end
4 changes: 4 additions & 0 deletions app/models/saved_search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ class SavedSearch < CaseflowRecord
validates :description, presence: true, length: { maximum: 1000 }
validate :saved_search_limit

scope :all_saved_searches, -> { order(created_at: :desc) }
almorbah marked this conversation as resolved.
Show resolved Hide resolved

scope :my_saved_searches, ->(user) { where(user_id: user).order(created_at: :desc) }
almorbah marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Active record is smart enough to realize that user_id is a foreign key and automatically transform user -> user.id, but I think it's probably better to make it more explicit in the scope. You can also chain scopes and if you keep the other scope you should chain it here instead of repeating .order(created_at: :desc). This is also why I don't think the name makes much sense when you look at it written out like this.

Suggested change
scope :my_saved_searches, ->(user) { where(user_id: user).order(created_at: :desc) }
scope :my_saved_searches, ->(user) { where(user: user).all_saved_searches }


private

def saved_search_limit
Expand Down
19 changes: 19 additions & 0 deletions app/serializers/saved_search_serializer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

class SavedSearchSerializer
include FastJsonapi::ObjectSerializer

attribute :name
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably also need to serialize the id since that will be what we are sending back to the backend when we are deleting a saved search.

attribute :description
attribute :saved_search
Copy link
Contributor

Choose a reason for hiding this comment

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

You can either manually write all these out as camelCased keys or use one of the key transform methods to do it automatically set_key_transform :camel_lower

Suggested change
attribute :saved_search
attribute :savedSearch

attribute :createdAt, &:created_at
attribute :userCssId do |object|
almorbah marked this conversation as resolved.
Show resolved Hide resolved
object.user.css_id
end
attribute :userFullName do |object|
Copy link
Contributor

Choose a reason for hiding this comment

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

These will error if there is no user. I would group them up as a user attribute. It would be similar to how the task serializer handles it's assigned to relationship. It's just an example so that the frontend redux store will be able to look for user attributes like this savedSearch.user.cssId

  # Example assigned_to from task serializer
  attribute :assigned_to do |object|
    assignee = object.try(:unscoped_assigned_to)

    {
      css_id: assignee.try(:css_id),
      full_name: assignee.try(:full_name),
      is_organization: assignee.is_a?(Organization),
      name: assignee.is_a?(Organization) ? assignee.name : assignee.css_id,
      status: assignee.try(:status),
      type: assignee.class.name,
      id: assignee.id
    }
  end

object.user.full_name
almorbah marked this conversation as resolved.
Show resolved Hide resolved
end
attribute :userId do |object|
object.user.id
end
end
15 changes: 15 additions & 0 deletions app/views/saved_searches/index.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<% content_for :full_page_content do %>
almorbah marked this conversation as resolved.
Show resolved Hide resolved
<%= react_component("NonComp", props: {
userDisplayName: current_user.display_name,
dropdownUrls: dropdown_urls,
applicationUrls: application_urls,
feedbackUrl: feedback_url,
buildDate: build_date,
flash: flash,
serverNonComp: {
currentUserCssId: current_user.css_id,
},
savedSearches: {
TylerBroyles marked this conversation as resolved.
Show resolved Hide resolved
}
}) %>
<% end %>
15 changes: 15 additions & 0 deletions app/views/saved_searches/show.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<% content_for :full_page_content do %>
<%= react_component("NonComp", props: {
userDisplayName: current_user.display_name,
dropdownUrls: dropdown_urls,
applicationUrls: application_urls,
feedbackUrl: feedback_url,
buildDate: build_date,
flash: flash,
serverNonComp: {
currentUserCssId: current_user.css_id,
},
savedSearches: {
TylerBroyles marked this conversation as resolved.
Show resolved Hide resolved
}
}) %>
<% end %>
8 changes: 4 additions & 4 deletions client/app/nonComp/pages/SavedSearches.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,19 @@ const SavedSearches = () => {

const ALL_TABS = [
{
key: 'my_saved_searches',
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't bother doing any minor tweaks to the javascript files in this PR that really only affect mocked data. Let's just keep it all backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pamatyatake2 let revert this change here

key: 'my_saved',
label: 'My saved searches',
// this section will later changed to backend call
page: <SearchTable
eventRows={savedSearchesData.savedSearches.rows.filter((rows) => rows.userCssId === currentUserCssId)}
eventRows={savedSearchesData.savedSearches.all.filter((rows) => rows.userCssId === currentUserCssId)}
searchPageApiEndPoint
/>
},
{
key: 'all_saved_searches',
key: 'all_saved',
label: 'All saved searches',
page: <SearchTable
eventRows={savedSearchesData.savedSearches.rows}
eventRows={savedSearchesData.savedSearches.all}
searchPageApiEndPoint
/>
}
Expand Down
5 changes: 4 additions & 1 deletion client/test/data/nonComp/savedSearchesData.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default {
savedSearches: {
status: 'idle',
error: null,
rows: [
all: [
{
id: 1,
name: 'Search Name',
Expand Down Expand Up @@ -279,6 +279,9 @@ export default {
}
}
}
],
my: [

],
fetchIndividualHistory: {
status: 'succeeded'
Expand Down
3 changes: 2 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,8 @@
end
end
get "report", to: "decision_reviews#generate_report", on: :member, as: :report, format: false
get "report/searches", to: "decision_reviews#generate_report", on: :member, as: :saved_searches, format: false
get "/(*all)", to: "decision_reviews#index"

end

resources :unrecognized_appellants, only: [:update] do
Expand All @@ -337,6 +337,7 @@
patch "/messages/:id", to: "inbox#update"
end

resources :saved_searches, only: [:index, :create, :destroy, :show]
resources :users, only: [:index, :update] do
resources :task_pages, only: [:index], controller: 'users/task_pages'
get 'represented_organizations', on: :member
Expand Down
136 changes: 136 additions & 0 deletions spec/controllers/saved_searches_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
# frozen_string_literal: true

describe SavedSearchesController, :postgres, type: :controller do
let(:user) { create(:user, :vha_admin_user, :with_saved_search_reports) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the :vha_admin_user trait already creates adds the user to the VhaBusienssLine so you probably don't have to add it later non_comp_org.add_user(user)

let(:saved_search) { create(:saved_search, user: user) }

let(:default_user) { create(:user) }
let(:vha_business_line) { VhaBusinessLine.singleton }
Copy link
Contributor

Choose a reason for hiding this comment

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

vha_business_line and non_comp_org can probably be combined into one variable since they are doing the same thing.

let(:options) { { format: :json } }

before do
User.stub = user
end

describe "#create" do
let(:valid_params) do
{
search: {
name: Faker::Name.name,
description: Faker::Lorem.sentence,
saved_search: {
report_type: "event_type_action",
events: {
"0": "added_issue_no_decision_date"
},
timing: {
range: "last_7_days"
},
decision_review_type: {
"0": "HigherLevelReview", "1": "SupplementalClaim"
},
business_line_slug: "vha"
}
}
}
end

context "VHA user creating saved search" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
context "VHA user creating saved search" do
context "VHA admin user creating saved search" do

it "should create search" do
expect { post :create, params: valid_params }
.to change(SavedSearch, :count).by(1)

expect(response).to have_http_status(:created)
end
end
end

describe "#destory /saved_searches/:id" do
context "VHA user saved search exists" do
it "should delete search" do
delete :destroy, params: { id: user.saved_searches.first.id }

expect(response).to have_http_status(:ok)
end
end

context "VHA user saved search not exists" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
context "VHA user saved search not exists" do
context "VHA admin user saved search not exists" do

it "retunrs a not found error" do
delete :destroy, params: { id: 0 }

expect(response).to have_http_status(:not_found)
end
end
end

describe "#index" do
before do
vha_business_line.add_user(default_user)
User.stub = default_user
end
context "get saved search" do
subject do
get :index,
params: options
end

context "user is not an vha admin" do
it "returns unauthorized" do
subject

expect(response.status).to be 302
expect(response.body).to match(/unauthorized/)
end
end

context "user is vha admin" do
before do
OrganizationsUser.make_user_admin(default_user, vha_business_line)
end

let!(:user_searches) { create_list(:saved_search, 5, user: default_user) }
let!(:other_user_searches) { create_list(:saved_search, 10) }

it "returns a successful response" do
subject

expect(response.status).to eq 200
expect(JSON.parse(response.body)["all_searches"].count).to eq(16)
expect(JSON.parse(response.body)["user_searches"].count).to eq(5)
end
end
end
end

describe "#show" do
before do
OrganizationsUser.make_user_admin(default_user, vha_business_line)
User.stub = default_user
end

context "get single saved search" do
let(:saved_search) do
create(:saved_search,
user: default_user,
name: "Timing Specification Before",
description: "Timing range before date")
end

subject do
get :show,
params: { id: saved_search.id, format: :json }
end

it "returns specific saved search" do
subject

expect(subject.response_code).to eq 200

response = JSON.parse(subject.body)

expect(response["attributes"]["name"]).to eq "Timing Specification Before"
expect(response["attributes"]["description"]).to eq "Timing range before date"
end
end
end
end
31 changes: 31 additions & 0 deletions spec/models/serializers/saved_search_serializer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# frozen_string_literal: true

describe SavedSearchSerializer, :postgres do
describe "#as_json" do
let(:user) { create(:user, :vha_admin_user) }
let(:saved_search) do
create(
:saved_search,
user: user,
name: "my_first_search",
description: "my first search",
saved_search: "{report_type: 'event_type_action'}"
)
end

subject { described_class.new(saved_search) }

it "renders saved search data" do
serializable_hash = {
name: "my_first_search",
description: "my first search",
saved_search: "{report_type: 'event_type_action'}",
createdAt: saved_search.created_at,
userCssId: user.css_id,
userFullName: user.full_name,
userId: user.id
}
expect(subject.serializable_hash[:data][:attributes]).to eq(serializable_hash)
end
end
end
Loading