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: migrate old custom Django message notifications #10988

Closed
humitos opened this issue Jan 3, 2024 · 2 comments · Fixed by #10994
Closed

Notifications: migrate old custom Django message notifications #10988

humitos opened this issue Jan 3, 2024 · 2 comments · Fixed by #10994
Assignees
Labels
Improvement Minor improvement to code

Comments

@humitos
Copy link
Member

humitos commented Jan 3, 2024

I kept the rendering for the default Django messages in the base template. However, we need to take a deeper look at it since it used to use django-messages-extends for extra and custom notifications (those that can be dismissed, etc) that we are not using anymore. That means the "Mark as read" URL and others won't work anymore.

Those messages have to be split into two groups:

  1. regular Django messages attached to the request
  2. notifications attached to the user that can be dismissed

<!-- BEGIN notify -->
{% block notify %}
{# TODO: migrate these notifications to new system #}
{% if messages %}
<ul class="notifications">
{% for message in messages %}
<li class="notification notification-{{ message.level }}" {% if message.pk %}data-dismiss-url="{% url 'message_mark_read' message.pk %}{% endif %}">
{% if message.pk %}
<a class="notification-action" href="{% url 'message_mark_read' message.pk %}">
<span class="icon close" aria-label="{% trans 'Close notification' %}"></span>
</a>
{% endif %}
{{ message }}
</li>
{% endfor %}
</ul>
{% endif %}
{% endblock %}
<!-- END notify -->

We talked about this on the PR while working on this: #10922 (comment)

@agjohnson
Copy link
Contributor

If I understand correctly, our plan on the old dashboard should be:

  • Anything left using Django messages at this point won't be changed to our new notification backend. These are form confirmations and other temporary Django messages.
  • We'd keep the template logic above for standard Django messages. The template is actually still accurate for standard messages (not sticky/persistent messages from messages-extends).
  • New Notification instances should use HTML similar to this template instead of separate HTML.

The new dashboard handles standard Django messages separately from sticky messages:

https://github.com/readthedocs/ext-theme/blob/bff4b4ddf645c9e5f690dd086f47602a74e7a429/readthedocsext/theme/templates/includes/utils/messages.html#L11-L45

That logic will need to be changed to:

  • Iterate over notifications to show Notification instances. The template logic will be changed slightly to accommodate the new Message class.
  • Iterate over messages to continue showing Django messages as toast messages. This logic doesn't change at all.

@github-project-automation github-project-automation bot moved this to Planned in 📍Roadmap Jan 3, 2024
@agjohnson agjohnson added the Improvement Minor improvement to code label Jan 3, 2024
@humitos
Copy link
Member Author

humitos commented Jan 4, 2024

Yes, I understand the summary of the plan you wrote is 👍🏼

The code will be simplified since message.pk won't exist anymore since there is no messages saved into the database anymore since we stopped used sticky/permanent messages via django-messages-extends.

Those sticky/permanent messages are now Notification objects attached to a particular resource. The code to render all of these does not exist yet and part of the work required for this issue.

humitos added a commit that referenced this issue Jan 4, 2024
Keep rendering regular one-time Django messages attached to the request, but
remove the logic used for sticky/permanent messages from
`django-messages-extends`.

Closes #10988
Related readthedocs/ext-theme#259
@humitos humitos moved this from Planned to Needs review in 📍Roadmap Jan 4, 2024
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Jan 4, 2024
humitos added a commit that referenced this issue Jan 4, 2024
Keep rendering regular one-time Django messages attached to the request, but
remove the logic used for sticky/permanent messages from
`django-messages-extends`.

Closes #10988
Related readthedocs/ext-theme#259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants