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: create notifications for projects with skip=True #10980

Closed
1 of 3 tasks
humitos opened this issue Jan 3, 2024 · 4 comments
Closed
1 of 3 tasks

Notifications: create notifications for projects with skip=True #10980

humitos opened this issue Jan 3, 2024 · 4 comments
Labels
Improvement Minor improvement to code

Comments

@humitos
Copy link
Member

humitos commented Jan 3, 2024

Projects with skip=True used to show a hard coded template notification at

{% if project.skip %}
<p class="build-failure">
{% blocktrans trimmed %}
Your project is currently disabled for abuse of the system.
Please make sure it isn't using unreasonable amounts of resources or triggering lots of builds in a short amount of time.
Please <a href="mailto:{{ SUPPORT_EMAIL }}">contact support</a> to get your project re-enabled.
{% endblocktrans %}
</p>
<br>
{% endif %}

Now, with the new notification system we have lost that and we have to:

  • create a Notification object for each of these projects currently skipped
  • create a Django signal to create a new Notification object when this field changes to skip=True
  • create a Django signal to cancel the Notification when this skip=False
@humitos
Copy link
Member Author

humitos commented Jan 3, 2024

I created a script at https://gist.github.com/humitos/b5fec6d741a19ee1762ebe39c7d203aa that migrates:

  • projects with skip=True and creates a Notification object for them
  • builds with .error field populated and creates a Notification object for them mapping the old -> new message

I tested this script locally and it worked fine. We should run it after performing the deploy of the new notification system. I focus only on all builds from 2023 and 2024. If we want to go further back in time, we will need to define more message mappings.

I'm not too worried about missing a mapping or similar since usually errors on old builds are not super useful, anyways.

@agjohnson agjohnson added the Improvement Minor improvement to code label Jan 3, 2024
@agjohnson
Copy link
Contributor

Also, we might not have a lot of project with this attribute. Even if we aren't doing a larger migration, we might just do a one-off solution here and make the necessary notification instances manually.

@humitos
Copy link
Member Author

humitos commented Jan 4, 2024

We talked in chat about this and we decided to only run the migration for Project.skip=True and keep the old Build.error messages around while maintaining the KO code to render them. This means reverting this commit: d4951a8 (#10922)

There won't be "duplicated notifications on builds" because the after merging #10922 we stop populating the Build.error field and start creating Notification objects instead. So, old builds will render the old field Build.error and new builds will render the new Notification objects 👍🏼

The only required part of the migration script is for Project.skip=True:

import structlog
from readthedocs.projects.notifications import MESSAGE_PROJECT_SKIP_BUILDS

log = structlog.get_logger(__name__)


# Migrate notifications for projects skipped
for project in Project.objects.filter(skip=True):
    log.info("Creating project skipped notification.", project_slug=project.slug)
    Notification.objects.add(
        attached_to=project,
        message_id=MESSAGE_PROJECT_SKIP_BUILDS,
        dismissable=False,
    )

In the future, if we change our mind or we want to go with the bigger Build.error -> Notification migration, we can do it using the script I wrote.

Slack conversation: https://readthedocs.slack.com/archives/C02DNG2HSNP/p1704296581578539?thread_ts=1704293584.321199&cid=C02DNG2HSNP

humitos added a commit that referenced this issue Jan 4, 2024
This reverts commit d4951a8.

We decided to keep this code around for now.
See #10980 (comment)
humitos added a commit that referenced this issue Jan 4, 2024
* Add some comments to understand the scope of the changes

* Add more comments about the plan

* Delete old code

We are not sending any kind of notification from the Django admin.
We used to use this in the past but we are not using it anymore.

I'm deleting it now to avoid re-implementing it using the new notification
system.

* Add more comments to organize the work of new notification system

* Notification: initial modelling

* Define `max_length=128` that's required for SQLite (tests)

* Initial implementation for build notifications API endpoint

* API endpoint to create build notifications

* Use APIv2 for internal endpoint to call from builders

* Use APIv2 to create notifications from the builder

* Check project slug when attaching to a Build/Project instance

* Add extra TODO comments

* Initial migration from Exceptions to Messages

* Disable `Notification.format_values` for now

I'll come back to this later.

* Define all the notifications

* Messages for symlink exceptions

* Simplify how `NOTIFICATION_MESSAGES` is built

* Add `Notification.format_values` field

This allows to store values required for this notification at render time

* Notification with `format_values` passed when exception is raised

* Use `get_display_icon` to show the proper icon in the serializer

* Proper relation name

* Initial work to replace site notifications

* We are not going to create Notifications via APIv3 for now

* Send `format_values` to exception

* Small changes and comments added

* Initial work to migrate an old SiteNotification to new system

* Create a notification `registry` to register messages by application

* Use a custom `models.Manager` to implement de-duplication

Use `Notification.objects.add()` to create a notification with de-duplication
capabilities. If the notification already exists, it updates all the fields with
the new ones and set the `modified` datetime to now.

This commit also migrates the "Your primary email address is not verified" to
the new notification system, which it was when I implemented the de-duplication logic.

* Small refactor to email notifications

* Render build/project notifications from Django templates

Use a small hack on build detail's page to refresh the page once the build is
finished. This way, we don't have to re-implement the new notification system in
the old Knockout javascript code and we focus ourselves only in the new
dashboard.

* Attach "build.commands" in beta notification

* Initial migration pattern for YAML config errors

* Pass default settings for notifications

* Organization disabled notification

* Email not verified notification

* Project skipped notification

* Missing file for NotificationQuerySet

* Migrate `oauth` app notifications to the new system

* Add small comment about notification migration

* Rename import

* Fix queryset when creating a notification via `.add()`

* Remove `SiteNotification` completely

* Rename file and remove `__init__` magic

* Remove `django-message-extends` completely

We are not going to use it anymore with the new notification system.

* Note about user notifications in base template

* Remove commented code for debugging

* Typo (from review)

* Migrate `ConfigError` to the new notification system

* Implement refact `icon` and `icon_type` into `icon_classes`

* Remove unused setting

* Update config file test cases to match changes

* Mark test that depend on search to be skipped

* Cancel build test fixed

* Update APIv3 test cases

* Down to a few of tests failing

* Raise exception properly by passing `message_id`

* More tests updated/fixed

* BuildUser/BuildAdd error

* Instantiate `BuildDirector` on `before_start`

It makes more sense to be there than under `execute()`, since it's a
pre-requirement. It also helps when testing.

* Update test case to pass

* Update more tests related to notifications

* Make usage of `.add` for deduplication

* Move the link to the message itself.

* Create message IDs property

* Use IDs to avoid notifications

* Update comments

* Attach organization on retry

* Test we are hitting APIv2 for notifications

* Remove old comment

* Remove temporary debugging link

* Remove old comment

* Update send build status celery tests

* Explain how to show notifications attached to users

* Better comment in template

* Lint

* Minor changes

* Refactor `ValidationError` to raise a proper exception/notification

* Update tests for config after refactor

* Pass `user_notifications` to templates

* More updates on config tests

* Remove debugging code

* Add context processor for user notifications

* Don't retrieve notifications if anonymous user

* Use proper exception

* Apply suggestions from code review

Co-authored-by: Anthony <aj@ohess.org>

* Remove old TODO messages

* Remove `deprecated_` utils and notifications

They are not useful anymore since it's not possible to build without a config
file or using a config file v1.

* Use the proper HTML/CSS for user notifications

* Message for deploy key (required in commercial)

* Migrate Docker errors to new system

* Migrate more messages to the new notifications system

* Typo

* Fix error type

* Remove showing errors from `build.error`

We are using the `Notification`s attached to the object now.
These old errors are migrated to the new system by a one-time script.

* Typo in copy

* Revert "Remove showing errors from `build.error`"

This reverts commit d4951a8.

We decided to keep this code around for now.
See #10980 (comment)

* Comment explaining the decision about `Build.error` HTML+KO code

* Use `textwrap.dedent` + `.strip()` for all messages

Closes #10987

* Avoid breaking at rendering time

If the message ID is not found we return a generic message to avoid breaking the
request and we log an internal error to fix the issue.

* Apply feedback from review

---------

Co-authored-by: Anthony <aj@ohess.org>
@humitos
Copy link
Member Author

humitos commented Jan 10, 2024

Projects with skip=True are already migrated to the new notification system.

@humitos humitos closed this as completed Jan 10, 2024
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

No branches or pull requests

2 participants