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

AF-156 Hide Tactic+Technique References from Flow #80

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

nikorev
Copy link
Contributor

@nikorev nikorev commented Jul 12, 2023

Currently works, but before merging I want to review the interaction of the diagram model typescript files and the vue components to ensure everything is being verified correctly. is_visible has been split to is_visible_chart and is_visible_sidebar to denote whether a property should be visible on the chart's node and/or property-editor sidebar.

Property declarations were updated in builder.config.ts to setup the schema, blocking the tactic+technique references from rendering on the chart while still being viewable/editable in the sidebar.

A couple discussion points I'd like to chat about pre-merge:

  1. BranchBlockModel.ts and DictionaryBlockModel.ts were both updated. For checking the visibility of these fields, I used is_visible_chart since this seemed to lay out the boilerplate which the vue-component would later use to populate the chart's/diagram's UI node with properties. Is this the right thinking?
  2. DictionaryField.vue vs DictionaryFieldContents.vue. How do each of these handle the rendering individual nodes to the screen? I was surprised to find success when verifying visibility with is_visible_chart with DictionaryField.vue, but is_visible_sidebar with DictionaryFieldContents.vue. I would think both of these would want to use is_visible_chart, but that wasn't the case.
  3. Property editor component (PropertyEditor.vue) verifies visibility using is_visible_sidebar, but the behavior didn't change when verifying with is_visible_chart. This should've technically made the technique_ref and tactic_ref disappear from the property editor sidebar, unless this is a change that needs to be made in EditorSidebar.vue. I swayed away from modifying EditorSidebar.vue for now since this didn't do any visibility checking previously; my "first pass" at this ticket was to transition existing is_visible calls to the new sidebar+chart solution. In summary, what is the relationship of PropertyEditor.vue vs EditorSidebar.vue?

@nikorev nikorev requested a review from mikecarenzo July 12, 2023 16:43
Copy link
Contributor

@mikecarenzo mikecarenzo left a comment

Choose a reason for hiding this comment

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

In retrospect, splitting is_visible into two configurations wasn't the best approach. is_visible_chart is a configuration that only applies to Dictionary Blocks and Branch Blocks. This configuration wouldn't have any effect on things like Text Blocks. It also directly conflicts with is_primary which effectively forces a property to be rendered, no matter what. We might want to consider a configuration on the Dictionary/Branch Block itself (something like listed_properties which explicitly lists which properties are rendered on the block).

To be clear, this doesn't need to be done in this PR, it's just something we should consider refactoring in the future.

@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
7.7% 7.7% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@mikecarenzo mikecarenzo marked this pull request as ready for review July 20, 2023 18:52
@mikecarenzo mikecarenzo merged commit 1230825 into main Jul 20, 2023
3 of 6 checks passed
@mikecarenzo mikecarenzo deleted the AF-156_hide_flow_references branch July 20, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants