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

feat: Site status message #7659

Merged
merged 31 commits into from
Aug 7, 2024
Merged

feat: Site status message #7659

merged 31 commits into from
Aug 7, 2024

Conversation

holloway
Copy link
Contributor

@holloway holloway commented Jul 10, 2024

Features

  • show a status message on all public pages
  • doesn't require JavaScript
    • non-JS browsers use an <iframe> pointing to a route showing the latest status message. If no site message is active it shows "No site status message.". The iframe is a <noscript><iframe src=></iframe></noscript> so only browser without JavaScript have this fallback.
    • JS-enabled browsers use Vue NaiveUI: Notifications with a default design. Dismissed statuses are noted in browser localStorage (ie per device and browser) not per user, so dismissed statuses aren't remembered on a different device and will need to be dismissed again.
  • doesn't pollute Cloudflare cache with status messages (ie status messages aren't rendered inline in each Django page as Cloudflare would cache outdated statuses... instead the browser composes the page with JavaScript+APIs or IFrames)
  • Has an endpoint to redirect users to the latest status: /status
  • Django and Playwright tests

@holloway holloway requested a review from rjsparks July 11, 2024 07:04
@@ -35,6 +35,7 @@ def status_latest_html(request):

def status_page(request, slug):
status = get_object_or_404(Status, slug=slug)
print("status", status)
Copy link
Member

Choose a reason for hiding this comment

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

debug leak?

@holloway
Copy link
Contributor Author

Hey @rjsparks this is ready for another review

Comment on lines 31 to 48
console.info("No status message")
return
}
const dismissedStatuses = getDismissedStatuses()
if(dismissedStatuses.includes(status.id)) {
console.info(`Not showing site status ${status.id} because it was already dismissed. Dismissed Ids:`, dismissedStatuses)
return
}

const isSameStatusPage = Boolean(document.querySelector(`[data-status-id="${status.id}"]`))

if(isSameStatusPage) {
console.info(`Not showing site status ${status.id} because we're on the target page`)
return
}

if(notificationInstances[status.id]) {
console.info(`Not showing site status ${status.id} because it's already been displayed`)
Copy link
Member

Choose a reason for hiding this comment

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

Change to console.debug?

@@ -0,0 +1,83 @@
<script>
Copy link
Member

Choose a reason for hiding this comment

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

Use <script setup> instead of manually defining the component further down.

Copy link
Member

Choose a reason for hiding this comment

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

Various uses of semi-colons. Remove all.

@@ -15,6 +16,7 @@ import NTheme from './components/n-theme.vue'
const availableComponents = {
ChatLog: defineAsyncComponent(() => import('./components/ChatLog.vue')),
Polls: defineAsyncComponent(() => import('./components/Polls.vue')),
Status: defineAsyncComponent(() => import('./components/Status.vue')),
Copy link
Member

Choose a reason for hiding this comment

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

No dangling comma

@@ -0,0 +1,18 @@
export const JSONWrapper = {
parse(jsonString, defaultValue) {
if(typeof jsonString !== "string") return defaultValue
Copy link
Member

Choose a reason for hiding this comment

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

Avoid single line if / return

@@ -0,0 +1,22 @@
# Copyright The IETF Trust 2017-2020, All Rights Reserved
Copy link
Member

Choose a reason for hiding this comment

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

this is a new model copyright should be 2024 - same with all other new files. Modified files should have the date range extended to 2024

@@ -0,0 +1,48 @@
# Generated by Django 4.2.13 on 2024-06-24 04:24
Copy link
Member

Choose a reason for hiding this comment

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

Replace the Generated by with the trust copyright line

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 is a new app coming in, delete the migrations and regenerate them so that there's only a 0001_initial.

@richsalz
Copy link
Collaborator

I wrote a tool for OpenSSL to do it automatically. Do you want me to open an issue to add something like that to the release process? https://github.com/openssl/tools/blob/master/release-tools/do-copyright-year

@rjsparks
Copy link
Member

Sure - split that into a separate issue so we don't distract from this PR too much.

Copy link
Member

@rjsparks rjsparks left a comment

Choose a reason for hiding this comment

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

Very close, but I've marked a few things I think need to change.

Comment on lines 14 to 16
status = Status.objects.order_by("-date").first()
if status is None or status.active == False:
return { "hasMessage": False }
Copy link
Member

Choose a reason for hiding this comment

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

This prevents us from staging the next message.
Consider instead:

    status = Status.objects.filter(active=True).order_by("-date").first()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. This was intentional (ie only the latest status which must also be active is used) but happy to change it.

Comment on lines 18 to 19
if status.slug is None:
raise FieldError("No slug generated. This shouldn't happen.")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is a valid use of FieldError. In any case there's nothing to catch this, so it would result in a 500 being served.

It would be better to make the slug a required and unique field in the model with null=False, blank-False, unique=True, with and let the ORM ensure it will never be empty,

"body": status.body,
"url": f"/status/{status.slug}",
"date": status.date.isoformat(),
"by": status.by.name,
Copy link
Member

Choose a reason for hiding this comment

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

by seems to be inconsistently used? The admin could default it to the logged in user, but maybe we should remove it from the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it from the templates / API response, but kept it in the model for auditing purposes. Happy to remove it if you'd prefer though.

<h1 data-status-id="{{ status.id }}">
{% block title %} {{ status.title }} {% endblock %}
</h1>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Since the view that uses this will let you look at inactive slugs, it would be good to put something in the template that warns if the status is no longer active.

Copy link
Contributor Author

@holloway holloway Jul 16, 2024

Choose a reason for hiding this comment

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

Good idea. I've added a Bootstrap 'badge' to the <h1> saying "inactive"... thoughts?

Screenshot from 2024-07-16 12-36-17

Copy link
Member

Choose a reason for hiding this comment

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

wfm

@holloway
Copy link
Contributor Author

@rjsparks @NGPixel ready for review again

@holloway holloway marked this pull request as ready for review July 17, 2024 20:19
@jennifer-richards
Copy link
Member

I think the failing URL test is caused by not covering the api/v1/(?P<api_name>status)/ endpoint

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

This is looking good. I have a few minor comments / change requests.

})

def status_latest_json(request):
return HttpResponse(json.dumps(get_context_data()), status=200, content_type='application/json')
Copy link
Member

Choose a reason for hiding this comment

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

There's a JsonResponse that might be more appropriate here

ietf/status/tests.py Outdated Show resolved Hide resolved
ietf/status/tests.py Outdated Show resolved Hide resolved
ietf/status/views.py Outdated Show resolved Hide resolved
ietf/status/views.py Outdated Show resolved Hide resolved
Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

lgtm. Only nit I'm noticing (which I should have noticed before) is that we have copyright year ranges going back to 2016 or 2017 on newly added files. In some cases maybe this is appropriate if it's copied code, but I have generally started with only the current year in a new file if it's only standard boilerplate carried over from other places.

Don't know if we're consistent enough about this for it to be worthwhile to change (or if it's even appropriate).

@holloway
Copy link
Contributor Author

@jennifer-richards i've updated the ©

rjsparks
rjsparks previously approved these changes Aug 1, 2024
@rjsparks rjsparks dismissed NGPixel’s stale review August 7, 2024 15:16

Noting change was made on Nick's behalf

@rjsparks rjsparks dismissed stale reviews from jennifer-richards and themself via b21661c August 7, 2024 15:47
@rjsparks rjsparks merged commit e5e6c9b into main Aug 7, 2024
7 checks passed
@rjsparks rjsparks deleted the feat/status-message branch August 9, 2024 14:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants