-
Notifications
You must be signed in to change notification settings - Fork 18
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
base: feature/APPEALS-45267
Are you sure you want to change the base?
Al/appeals 45664 #23279
Conversation
Code Climate has analyzed commit 010e3c8 and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left a lot of comments around around naming conventions and small things like that so if you have any questions let me know.
@@ -18,19 +18,19 @@ const SavedSearches = () => { | |||
|
|||
const ALL_TABS = [ | |||
{ | |||
key: 'my_saved_searches', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
app/models/saved_search.rb
Outdated
@@ -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) } | |||
|
|||
scope :my_saved_searches, ->(user) { where(user_id: user).order(created_at: :desc) } |
There was a problem hiding this comment.
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.
scope :my_saved_searches, ->(user) { where(user_id: user).order(created_at: :desc) } | |
scope :my_saved_searches, ->(user) { where(user: user).all_saved_searches } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I noticed a couple of small things around the index/show html methods and some cleanup that we might want to address.
app/models/saved_search.rb
Outdated
@@ -7,6 +7,9 @@ class SavedSearch < CaseflowRecord | |||
validates :description, presence: true, length: { maximum: 1000 } | |||
validate :saved_search_limit | |||
|
|||
# Ex:- scope :active, -> {where(:active => true)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Ex:- scope :active, -> {where(:active => true)} |
attribute :userCssId do |object| | ||
object.user.css_id | ||
end | ||
attribute :userFullName do |object| |
There was a problem hiding this comment.
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
private | ||
|
||
def organization | ||
@organization ||= BusinessLine.find_by(url: params[:decision_review_business_line_slug]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decision_review_business_line_slug is not included in any of your routes so it's not going to know what this :decision_review_business_line_slug param is. It would have to be using the :business_line_slug param from the routes.
Something like this might work
@organization ||= Organization.find_by(url: params[:business_line_slug] || params[:organization_slug])
# frozen_string_literal: true | ||
|
||
describe SavedSearchesController, :postgres, type: :controller do | ||
let(:user) { create(:user, :vha_admin_user, :with_saved_search_reports) } |
There was a problem hiding this comment.
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(:non_comp_org) { VhaBusinessLine.singleton } | ||
|
||
let(:default_user) { create(:user) } | ||
let(:vha_business_line) { VhaBusinessLine.singleton } |
There was a problem hiding this comment.
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.
} | ||
end | ||
|
||
context "VHA user creating saved search" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context "VHA user creating saved search" do | |
context "VHA admin user creating saved search" do |
end | ||
end | ||
|
||
context "VHA user saved search not exists" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context "VHA user saved search not exists" do | |
context "VHA admin user saved search not exists" do |
config/routes.rb
Outdated
@@ -308,6 +308,7 @@ | |||
get '/remands(/*path)', to: redirect('/supplemental_claims/%{path}') | |||
|
|||
resources :decision_reviews, param: :business_line_slug do | |||
resources :saved_searches, only: [:index, :create, :destroy, :show] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still does not match the current SavedSearches page url which is /decision_reviews/business_line_slug/report/searches
. This route would match /decision_reviews/business_line_slug/searches
. Is that the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed one last thing, but that should be it.
class SavedSearchSerializer | ||
include FastJsonapi::ObjectSerializer | ||
|
||
attribute :name |
There was a problem hiding this comment.
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.
Resolves APPEALS-45664
Description
Please explain the changes you made here.
saved_searches_controller.rb
and its actions.index, show, delete and create
.saved_searches_controller_spec.rb
Acceptance Criteria
Testing Plan
In Ruby Console:
Tester can use Postman using this link with pre-saved URL endpoint criteria to ease help with testing.
In order to make an API call tester needs to capture
_caseflow_session
from the Chrome browser after logging in through the frontend with the type of user they want to test the endpoint with e.g vha adminFrontend
User Facing Changes
Storybook Story
For Frontend (Presentation) Components
MyComponent.stories.js
alongsideMyComponent.jsx
)Backend
Database Changes
Only for Schema Changes
created_at
,updated_at
) for new tablesCaseflow::Migration
, especially when adding indexes (useadd_safe_index
) (see Writing DB migrations)migrate:rollback
works as desired (change
supported functions)make check-fks
; add any missing foreign keys or add toconfig/initializers/immigrant.rb
(see Record associations and Foreign Keys)belongs_to
for associations to enable the schema diagrams to be automatically updatedIntegrations: Adding endpoints for external APIs
Best practices
Code Documentation Updates
Tests
Test Coverage
Did you include any test coverage for your code? Check below:
Code Climate
Your code does not add any new code climate offenses? If so why?
Monitoring, Logging, Auditing, Error, and Exception Handling Checklist
Monitoring
Logging
Auditing
Error Handling
Exception Handling