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

Added security watchtower page #35400

Merged
merged 7 commits into from
Dec 3, 2024
Merged

Added security watchtower page #35400

merged 7 commits into from
Dec 3, 2024

Conversation

mjriley
Copy link
Contributor

@mjriley mjriley commented Nov 19, 2024

Product Description

Added a new page for the security watchtower
image

Technical Summary

This is currently an empty page. It is awaiting new tiles that will be implemented as part of our enterprise console improvements.

Feature Flag

This is hidden behind the ENTERPRISE_DASHBOARD_IMPROVEMENTS feature flag.

Safety Assurance

Safety story

This is currently an empty page hidden behind a user flag that no users should have enabled. I verified the changes to the enterprise console page locally, testing that the page continued to display correctly and also verified generating the domain report.

Automated test coverage

No tests

QA Plan

No QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@mjriley mjriley added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Nov 19, 2024
Base automatically changed from mjr/enterprise_console_ff to master November 20, 2024 15:40
@biyeun
Copy link
Member

biyeun commented Nov 20, 2024

The breadcrumbs should be updated to show

Enterprise Console / Security Watchtower

And relatedly

Enterprise Console / Enterprise Dashboard

See this utility

def get_page_context(page_title, page_url, page_name=None, parent_pages=None, domain=None, section=None):

(not a blocker: can be part of this or a small followup PR)

@@ -191,30 +191,35 @@ hqDefine("enterprise/js/enterprise_dashboard", [
}

$(function () {
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to make this an es6 / esm module and use js_entry

Copy link
Member

Choose a reason for hiding this comment

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

Also, since this entry point will be used for both enterprise_dashboard and security_watchtower, would be good to rename the file so that it's clear it's not specific to either. e.g. enterprise_dashboard => enterprise_report_tiles?

Alternative...rename enterprise dashboard page to project health metrics?

Copy link
Member

Choose a reason for hiding this comment

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

Or "Project Overview"?

Copy link
Member

Choose a reason for hiding this comment

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

Or "Enterprise Overview"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in f35b96f

@@ -2,7 +2,7 @@
{% load hq_shared_tags %}
{% load i18n %}

{% block page_title %}{{ account.name }}{% endblock %}
{% block page_title %}{{ header_title }}{% endblock %}

{% requirejs_main_b5 'enterprise/js/enterprise_dashboard' %}
Copy link
Member

Choose a reason for hiding this comment

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

please migrate to webpack thank you!

Copy link
Member

Choose a reason for hiding this comment

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

requirejs_main_b5 => js_entry

and then

import 'commcarehq`;

at the top of the esm module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given work like #35409, this is going to cause conflicts. I think we should handle this as a separate PR

const maxDateRangeDays = initialPageData.get("max_date_range_days");
dateRangeModal = DateRangeModal(datePicker, dateRangePresetOptions, maxDateRangeDays, formSubmissionsDisplay);

$("#dateRangeDisplay").koApplyBindings(formSubmissionsDisplay);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$("#dateRangeDisplay").koApplyBindings(formSubmissionsDisplay);
$dateRangeDisplay.koApplyBindings(formSubmissionsDisplay);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9519faf

if not has_privilege(request, privileges.PROJECT_ACCESS):
return HttpResponseRedirect(reverse(EnterpriseBillingStatementsView.urlname, args=(domain,)))

context = {
Copy link
Member

Choose a reason for hiding this comment

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

would be good to use

def get_page_context(page_title, page_url, page_name=None, parent_pages=None, domain=None, section=None):

Copy link
Member

Choose a reason for hiding this comment

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

use page_title and page_name

page_name is "Security Watchtower for <request.account.name>"

and then specify a Section so the breadcrumbs work properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 54bb2aa

@@ -2,7 +2,7 @@
{% load hq_shared_tags %}
{% load i18n %}

{% block page_title %}{{ account.name }}{% endblock %}
{% block page_title %}{{ header_title }}{% endblock %}
Copy link
Member

@biyeun biyeun Nov 20, 2024

Choose a reason for hiding this comment

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

current_page.name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 54bb2aa

@jingcheng16
Copy link
Contributor

@mjriley All changes looks good
If I remember correctly, in last group review session, we agreed on implementing a partial template for the tile?

@mjriley
Copy link
Contributor Author

mjriley commented Dec 2, 2024

@mjriley All changes looks good If I remember correctly, in last group review session, we agreed on implementing a partial template for the tile?

Should be addressed in 63d6b28

@jingcheng16
Copy link
Contributor

@mjriley Thank you! Since data range modal is only used in tile, should it be moved into the partial template too?
{% include 'enterprise/partials/date_range_modal.html' %}

Is there anything below can be moved to the partial template?

  {% registerurl "enterprise_dashboard_email" domain "---" %}
  {% registerurl "enterprise_dashboard_total" domain "---" %}
  {% initial_page_data 'max_date_range_days' max_date_range_days %}
  {% initial_page_data 'metric_type' metric_type %}

@mjriley
Copy link
Contributor Author

mjriley commented Dec 3, 2024

@mjriley Thank you! Since data range modal is only used in tile, should it be moved into the partial template too? {% include 'enterprise/partials/date_range_modal.html' %}

Is there anything below can be moved to the partial template?

  {% registerurl "enterprise_dashboard_email" domain "---" %}
  {% registerurl "enterprise_dashboard_total" domain "---" %}
  {% initial_page_data 'max_date_range_days' max_date_range_days %}
  {% initial_page_data 'metric_type' metric_type %}

I think that might create undesired behavior? The template is going to be included for every report. If we were to include the modal or register the URLs within this partial template, I believe that would cause us to generate a separate modal and register separate URLs for each time the partial was included.

@jingcheng16
Copy link
Contributor

@mjriley Interesting! I didn't think about that! Thank you!

@mjriley mjriley merged commit 23682ba into master Dec 3, 2024
13 checks passed
@mjriley mjriley deleted the mjr/security_watchtower branch December 3, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants