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

Fix bug where we can't change tab from attack to another tab in Configure #1331

Merged
merged 3 commits into from
Jul 19, 2021

Conversation

ilija-lazoroski
Copy link
Contributor

@ilija-lazoroski ilija-lazoroski commented Jul 16, 2021

What does this PR do?

Fixes #1293 .

Add any further explanations here.

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by {Running the Monkey locally with relevant config/running Island/...}

  • If applicable, add screenshots or log transcripts of the feature working

Explain Changes

Note: I didn't do the test on Network or Ransomware. Check the fix in #1330 .

AttackBug

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #1331 (491c44a) into develop (c760e06) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1331   +/-   ##
========================================
  Coverage    40.25%   40.25%           
========================================
  Files          474      474           
  Lines        13860    13860           
========================================
  Hits          5579     5579           
  Misses        8281     8281           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c760e06...491c44a. Read the comment docs.

@VakarisZ
Copy link
Contributor

This is the issue #1293, right?

@ilija-lazoroski
Copy link
Contributor Author

This is the issue #1293, right?

Yep, changed it.

@@ -234,7 +236,11 @@ class ConfigurePageComponent extends AuthComponent {

updateConfigSection = () => {
let newConfig = this.state.configuration;
if (Object.keys(this.state.currentFormData).length > 0) {

if (this.currentSection === 'attack' && this.state.currentFormData === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check if this.state.currentFormData === undefined? how did it get undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undefined

Line 348. Because the key is attack. It is setting the currentFormData to this.state.configuration[key] which is undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe when setting the attack tab we should set form data to {} so the check of Object.keys(this.state.currentFormData).length > 0 would work again?

if (Object.keys(this.state.currentFormData).length > 0) {

if (this.currentSection === 'attack' && this.state.currentFormData === undefined) {
this.state.currentFormData = this.state.attackConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this.state.currentFormData to be set to this.state.attackConfig, because this.state.attackConfig; is a separate thing. On attack matrix we don't submit the form data, thus we don't need to track it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then maybe set it to empty ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I've tried it, but still run into bugs

@ilija-lazoroski ilija-lazoroski force-pushed the 1293/attack-no-change-tabs branch 2 times, most recently from f2caa09 to 1eb0c7b Compare July 19, 2021 08:54
@VakarisZ VakarisZ force-pushed the 1293/attack-no-change-tabs branch from 1eb0c7b to 491c44a Compare July 19, 2021 09:24
@VakarisZ VakarisZ merged commit b798e67 into develop Jul 19, 2021
@VakarisZ VakarisZ deleted the 1293/attack-no-change-tabs branch July 19, 2021 10:59
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.

Can't change config tabs if reset/submit/import is done on ATT&CK config tab
2 participants