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

Migrate alert management to Wagtail #1174

Merged
merged 11 commits into from
Dec 11, 2024
Merged

Migrate alert management to Wagtail #1174

merged 11 commits into from
Dec 11, 2024

Conversation

hancush
Copy link
Collaborator

@hancush hancush commented Nov 22, 2024

Overview

See title. Connects #1175.

Demo

Screenshot 2024-11-22 at 9 58 46 AM Screenshot 2024-12-04 at 1 34 07 PM Screenshot 2024-12-04 at 1 34 14 PM Screenshot 2024-12-04 at 1 34 59 PM

Testing Instructions

Credentials for the datamade user are in Bitwarden under LA Metro Wagtail User - Staging

  • Using the review app, add some alerts with various rich text elements. Confirm they appear on the site as expected.
  • Also confirm that there's a link to manage alerts in the Wagtail userbar in the bottom left.

FieldPanel("description", widget=MarkdownTextarea),
]

def content(self):
Copy link
Collaborator Author

@hancush hancush Nov 22, 2024

Choose a reason for hiding this comment

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

The ModelAdmin can render custom model methods in the list display. This method renders the alert description and adds a styled label for the alert type.

Screenshot 2024-11-22 at 10 04 39 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah neat, and TIL format_html and expand_db_html are useful tools to return db stored html

@@ -1237,17 +1237,3 @@ def __str__(self):

else:
return self.name


class Alert(models.Model):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Migrated verbatim to models/cms.py.

@@ -0,0 +1,53 @@
[class*="alert"] {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Low key obsessed with CSS variables.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did some reading up on this and oh yea I can totally see why 👀

Comment on lines +17 to +37
// Callback function to execute when mutations are observed
const styleAlertTypeSelect = (mutationList, observer) => {
mutationList.map(mu => {
mu.addedNodes && mu.addedNodes.forEach(node => {
if (node.id === "id_type") {
styleParent({target: node})
node.addEventListener("change", styleParent)
observer.disconnect()
return
}
})
})
};

// Create an observer instance linked to the callback function
const observer = new MutationObserver(styleAlertTypeSelect);

const config = {childList: true, subtree: true};

// Start observing the target node for configured mutations
observer.observe(document.body, config);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This whole rigamarole is necessary because Wagtail adds form fields to the DOM in JavaScript. In order to update the alert type select, we have to wait for it to show up, give it an initial style, then add an event listener for changes. The MutationObserver API is pretty dope: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

h/t to @xmedr for this UI!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah so is the mutation observer being used here mostly because we can't attach an event listener the usual way since the selector doesn't exist yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly!

@@ -115,6 +97,8 @@
{% endblock %}
</main>

{% wagtailuserbar 'bottom-left' %}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaces conditional admin link in top nav.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since they'll have to sign in to wagtail to manage the alerts after this, would it be useful to change up the Metro Login link in the footer? Maybe by making it link to the wagtail login instead. That is, as long as logging in through wagtail would still allow them to manage things like manual broadcast links or events that don't appear in legistar, which I imagine it would.

If we think yes, then I can take care of it when I branch off of this!

(Also something to consider in the future is whether wagtail should also somehow manage manual broadcasts.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good idea. Yes!

)


@hooks.register("construct_wagtail_userbar")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

return format_html('<script src="{}"></script>', static("js/wagtail_custom.js"))


@hooks.register("insert_global_admin_css", order=500)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

return items


@hooks.register("insert_global_admin_js", order=100)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -15,5 +15,5 @@ repos:
rev: '9f60881'
hooks:
- id: flake8
files: lametro councilmatic tests
files: ^(lametro|councilmatic|tests)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TRICKY LITTLE BUGGER

@hancush hancush temporarily deployed to la-metro-cou-hcg-alerts-dpuhsl November 22, 2024 16:35 Inactive
@hancush hancush temporarily deployed to la-metro-cou-hcg-alerts-dpuhsl December 4, 2024 19:32 Inactive
@hancush hancush marked this pull request as ready for review December 4, 2024 19:41
@hancush hancush requested a review from antidipyramid December 4, 2024 19:41
@hancush hancush temporarily deployed to la-metro-cou-hcg-alerts-dpuhsl December 4, 2024 19:43 Inactive
@xmedr xmedr requested review from xmedr and removed request for antidipyramid December 4, 2024 19:46
Copy link
Collaborator

@xmedr xmedr left a comment

Choose a reason for hiding this comment

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

This works great! Do you think it'd be worth to add a border to the type select? I had trouble noticing it at first with a light theme and nothing selected (pic below). Otherwise, I've just got a couple questions inline.

image

FieldPanel("description", widget=MarkdownTextarea),
]

def content(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah neat, and TIL format_html and expand_db_html are useful tools to return db stored html

@@ -0,0 +1,53 @@
[class*="alert"] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did some reading up on this and oh yea I can totally see why 👀

@@ -115,6 +97,8 @@
{% endblock %}
</main>

{% wagtailuserbar 'bottom-left' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since they'll have to sign in to wagtail to manage the alerts after this, would it be useful to change up the Metro Login link in the footer? Maybe by making it link to the wagtail login instead. That is, as long as logging in through wagtail would still allow them to manage things like manual broadcast links or events that don't appear in legistar, which I imagine it would.

If we think yes, then I can take care of it when I branch off of this!

(Also something to consider in the future is whether wagtail should also somehow manage manual broadcasts.)

Comment on lines 41 to 44
def add_modeladmin_links(request, items):
for modeladmin_cls in (AlertAdmin,):
items.append(ModelAdminLink(modeladmin_cls))
return items
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why this is a loop, could you talk a bit about that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Factored out.

Comment on lines +17 to +37
// Callback function to execute when mutations are observed
const styleAlertTypeSelect = (mutationList, observer) => {
mutationList.map(mu => {
mu.addedNodes && mu.addedNodes.forEach(node => {
if (node.id === "id_type") {
styleParent({target: node})
node.addEventListener("change", styleParent)
observer.disconnect()
return
}
})
})
};

// Create an observer instance linked to the callback function
const observer = new MutationObserver(styleAlertTypeSelect);

const config = {childList: true, subtree: true};

// Start observing the target node for configured mutations
observer.observe(document.body, config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah so is the mutation observer being used here mostly because we can't attach an event listener the usual way since the selector doesn't exist yet?

@hancush hancush temporarily deployed to la-metro-cou-hcg-alerts-dpuhsl December 10, 2024 19:43 Inactive
@hancush hancush temporarily deployed to la-metro-cou-hcg-alerts-dpuhsl December 10, 2024 19:45 Inactive
@hancush hancush requested a review from xmedr December 10, 2024 19:45
Copy link
Collaborator

@xmedr xmedr left a comment

Choose a reason for hiding this comment

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

Awesome, looks good!

Comment on lines +14 to +17
/* Add default border to empty alert select */
.alert- {
border: 1px solid var(--w-color-border-field-default)!important;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, tyty!

@hancush hancush merged commit 4b4cad7 into hcg/wagtail Dec 11, 2024
2 checks passed
@hancush hancush deleted the hcg/alerts branch December 11, 2024 14:30
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