-
Notifications
You must be signed in to change notification settings - Fork 16
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
348 alternative testing #546
Conversation
PR this because it's much cleaner to focus only on the component. |
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.
Line 295: General rule, if you are uncertain whether the record exists or not, use find_by
and handle the nil case, and if you are confident that the record exists, use find
.
using find
raises an exception if the record is not found, which can impact performance and create unnecessary overhead in the application, even though the exception is handled.
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.
Also, we can't see the rule direct link when on edit mode. not sure if that's the intent.
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 rule direct link is at the bottom of the page, not quite noticeable. I think it will be better to have it at the top.
@@ -24,6 +25,34 @@ def index | |||
@components_json = Component.eager_load(:based_on).where(released: true).to_json | |||
end | |||
|
|||
def set_rule |
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 must be a private method
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
Also, during testing we discovered that it appears we've broken the top right "Search by SRG Version" functionality. It still searches, and navigates when selecting an item, but the selected item is not "pre-selected" like it used to be. |
looks clean to me |
Signed-off-by: Vanessa Fotso <vfotso@mitre.org>
…lcan into 348-alternative-testing
c3577dc
to
ad034e3
Compare
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.
Approving for deep linking functionality.
No description provided.