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 notifier without any layout #826

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 66 additions & 73 deletions services/notification/notifiers/mixins/message/__init__.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import logging
from typing import Callable, List
from typing import Callable
Swatinem marked this conversation as resolved.
Show resolved Hide resolved

from shared.django_apps.core.models import Repository
from shared.reports.resources import ReportTotals
from shared.validation.helpers import LayoutStructure

from database.models.core import Owner
from helpers.environment import is_enterprise
from helpers.metrics import metrics
from services.billing import BillingPlan
from services.comparison import ComparisonProxy, FilteredComparison
from services.notification.notifiers.mixins.message.helpers import (
Expand Down Expand Up @@ -202,98 +201,92 @@
def _team_plan_notification(
self,
comparison: ComparisonProxy,
message: List[str],
message: list[str],
diff,
settings,
links,
current_yaml,
) -> List[str]:
) -> list[str]:
writer_class = TeamPlanWriter()

with metrics.timer(
f"worker.services.notifications.notifiers.comment.section.{writer_class.name}"
):
# Settings here enable failed tests results for now as a new product
message.extend(
line
for line in writer_class.header_lines(
comparison=comparison, diff=diff, settings=settings
)
# Settings here enable failed tests results for now as a new product
giovanni-guidini marked this conversation as resolved.
Show resolved Hide resolved
message.extend(
writer_class.header_lines(
comparison=comparison, diff=diff, settings=settings
)
message.extend(
line
for line in writer_class.middle_lines(
comparison=comparison,
diff=diff,
links=links,
current_yaml=current_yaml,
)
)
message.extend(
writer_class.middle_lines(
comparison=comparison,
diff=diff,
links=links,
current_yaml=current_yaml,
)
message.extend(line for line in writer_class.footer_lines())
)
message.extend(writer_class.footer_lines())

return message
return message

def write_section_to_msg(
self, comparison, changes, diff, links, write, section_writer, behind_by=None
):
wrote_something: bool = False
with metrics.timer(
f"worker.services.notifications.notifiers.comment.section.{section_writer.name}"

for line in section_writer.write_section(
comparison, diff, changes, links, behind_by=behind_by
):
for line in section_writer.write_section(
comparison, diff, changes, links, behind_by=behind_by
):
wrote_something |= line is not None
write(line)
wrote_something |= line is not None
write(line)

if wrote_something:
write("")

def get_middle_layout_section_names(self, settings):
sections = map(
lambda layout: layout.strip(), (settings["layout"] or "").split(",")
)
return [
section
for section in sections
if section
not in [
"header",
"newheader",
"newfooter",
"newfiles",
"condensed_header",
"condensed_footer",
"condensed_files",
]
]

def get_upper_section_names(self, settings):
sections = list(
map(lambda layout: layout.strip(), (settings["layout"] or "").split(","))
)
headers = ["newheader", "header", "condensed_header"]
if all(x not in sections for x in headers):
def get_middle_layout_section_names(self, settings: dict) -> list[str]:
filtered_sections = {
"header",
"newheader",
"newfooter",
"newfiles",
"condensed_header",
"condensed_footer",
"condensed_files",
}

sections = get_sections(settings)

return [section for section in sections if section not in filtered_sections]

def get_upper_section_names(self, settings: dict) -> list[str]:
headers = {"newheader", "header", "condensed_header"}
allowed_sections = {
"header",
"newheader",
"condensed_header",
"newfiles",
"condensed_files",
}

sections = get_sections(settings)

if set(sections).isdisjoint(headers):
sections.insert(0, "condensed_header")

if "files" in sections or "tree" in sections:
sections.append("newfiles")

return [
section
for section in sections
if section
in [
"header",
"newheader",
"condensed_header",
"newfiles",
"condensed_files",
]
]

def get_lower_section_name(self, settings):
if (
"newfooter" in settings["layout"]
or "condensed_footer" in settings["layout"]
):
return [section for section in sections if section in allowed_sections]

def get_lower_section_name(self, settings: dict) -> str | None:
sections = get_sections(settings)

if "newfooter" in sections or "condensed_footer" in sections:
return "newfooter"
return None


def get_sections(settings: dict) -> list[str]:
layout = (settings.get("layout") or "").strip()
if not layout:
return []

Check warning on line 290 in services/notification/notifiers/mixins/message/__init__.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/notification/notifiers/mixins/message/__init__.py#L290

Added line #L290 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you return nothing the PR comment would be empty.
It's better to return the default value in case there is none.

The default value is here (and it should not be lost when the YAML is processed, but merged with the user's option - I think that's the real bug)


return [section.strip() for section in layout.split(",")]
Loading