Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 13 commits
1362d80
09786a0
e2a783a
abda93f
e94b7ea
359e869
66d4b4c
7442c91
1f2bc1b
05602da
010e3c8
58b2926
1db4af5
b91c2ff
edc3462
cd81930
60c0d75
2870721
8009c08
f762d04
e9c9a48
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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
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
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
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)
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.
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.
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.