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

Notifications Overhaul #1158, Enhancement #1154, and more.... #1437

Merged
merged 12 commits into from
Aug 2, 2019
Merged

Notifications Overhaul #1158, Enhancement #1154, and more.... #1437

merged 12 commits into from
Aug 2, 2019

Conversation

devEricA-zz
Copy link

This is a series of changes that I made to Defectdojo's Notifications as a response to Issue #1158

The following notifications were changed:

  • Review Requested : Now displays the requested users that are intended to review a finding.
  • Engagement Added: Now displays the engagement's name instead of the engagement's product.
  • Import Scan Results: Now Displays the scan that was uploaded, used to only display "More information can be found here: ....."
  • Test Added: Now displays the test's title along with the test's type.
  • Notes: Description changed from "User mentioned you in a note" to "User jotted a note"
  • Add Product: Now displays the product that was created, used to only display "More information can be found here....."

The following events now trigger a notification:

  • Deletion of Endpoints
  • Deletion of Engagements
  • Closure of Engagements
  • Reopening of Engagements
  • Addition of Findings
  • Deletion of Findings
  • Closure of Findings
  • Reopening of Findings
  • Deletion of Products
  • Deletion of Product Types
  • Deletion of Tests

Are there any other events that I should tag a notification to?

@madchap
Copy link
Contributor

madchap commented Jul 31, 2019

I think this is a great start, thank you!

  • Review Requested : Now displays the requested users that are intended to review a finding

So, everyone still gets the email, only the intended recipient is explicitly mentioned correct?

@devEricA-zz
Copy link
Author

devEricA-zz commented Jul 31, 2019

To the best of my knowledge, yes @madchap. Unfortunately, due to issue #1423, I can't test the email notifications to verify that. It does work in slack, where everyone gets the notification, but only the intended recipient(s) of the issue are mentioned.

@devEricA-zz devEricA-zz changed the title [In Progress] Notifications Overhaul #1158 Notifications Overhaul #1158 Jul 31, 2019
@devEricA-zz
Copy link
Author

Managed to also knock out #1154. A name field was created to allow a unique distinction between jira configurations.

@devEricA-zz devEricA-zz changed the title Notifications Overhaul #1158 Notifications Overhaul #1158 and Enhancement #1154 Jul 31, 2019
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

<option value="High">High</option>
<option value="Critical">Critical</option>
</select><br/><br/>
<b>Status</b> <input id="id_status" name="status" type="checkbox" alt="Select to enable"/><br/>
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

<span>Mitigated</span>
</label>
{% if "enable_jira"|get_system_setting %}
<b>Push to JIRA</b> <input id="id_push_tojira" name="push_to_jira" type="checkbox" alt="Select to push to JIRA"/><br/>
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

{% endfor %}`
</select>
</label>
<input type="submit" class="btn btn-sm btn-primary" value="Submit"/>
Copy link

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

👏 You fixed the issue(s)! Great work.

@devEricA-zz
Copy link
Author

These commits are for #1438 and #1281. Retire.js scan was fixed for the given file provided in #1438, and a comment icon appears for any findings with notes. Hovering over the commenting Icon displays the most recent note for the finding.

Additionally, the Bulk edit menus and page numbering for view test and findings list were re-positioned. The bulk edit menu for view test will now display inside the finding window, and the page number widget in view test's finding window was moved to the top. The bulk edit menu for findings list now appears under the page number widget.

NOTE: The Retire.js Sample Scan file provided in the sample scan repo continues to break when uploaded with Retire.js selected. I am in the process of replacing that file with the one provided in #1438 .

This will be the last commit I will make to this request, unless if the Travis CI Fails.

@devEricA-zz devEricA-zz changed the title Notifications Overhaul #1158 and Enhancement #1154 Notifications Overhaul #1158, Enhancement #1154, and more.... Aug 1, 2019
@devGregA devGregA merged commit cdf202b into DefectDojo:dev Aug 2, 2019
@Maffooch Maffooch mentioned this pull request Aug 6, 2019
3 tasks
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.

5 participants