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: Unified Notifications #290

Open
wants to merge 66 commits into
base: main
Choose a base branch
from

Conversation

jagadeeswaran-zipstack
Copy link
Contributor

@jagadeeswaran-zipstack jagadeeswaran-zipstack commented Apr 25, 2024

What

Unified Notifications consists of the following changes:

  1. Merging both log and notification messages to display them in the bottom logs component.
  2. Persisting both log and notification messages in Redis with a TTL of 1 hour.
  3. Making the logs component resizable, allowing users to drag and resize the component.
  4. Improving the styling of the logs component.

Why

The unified notification changes were implemented to enhance UX. Users can now see log and notification messages in one place, and these messages are persisted for 1 hour.

How

  1. Used Redis to store log and notification messages.
  2. Improved the styling of the components by using Antd components and additional custom styling.

Can this PR break any existing features. If yes please list of possible items. If no please exaplin why. (PS: Admins do not merge the PR without this section filled)

Yes, this PR can break existing features. The code changes have been made in quite a few existing modules, increasing the chances of regression.

Database Migrations

NA

Env Config

LOGS_EXPIRATION_TIME_IN_SECOND - This env variable stores the TTL in seconds. The default value is 3600 (1 hour).

Relevant Docs

NA

Related Issues or PRs

NA

Dependencies Versions

NA

Notes on Testing

NA

Screenshots

image
image
image

Checklist

I have read and understood the Contribution Guidelines.

Copy link

sonarcloud bot commented Jun 24, 2024

Copy link
Contributor

@muhammad-ali-e muhammad-ali-e left a comment

Choose a reason for hiding this comment

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

LGTM. However, I have a few suggestions:

  • Performance Considerations: This implementation processes a large number of keys for each session, which might lead to performance issues.
  • Feature Flag: To mitigate potential impacts, consider implementing this under a feature flag. This allows for controlled rollouts and easy rollback if necessary.

backend/backend/settings/base.py Outdated Show resolved Hide resolved
backend/logs_helper/log_service.py Outdated Show resolved Hide resolved
backend/logs_helper/views.py Show resolved Hide resolved
backend/logs_helper/views.py Outdated Show resolved Hide resolved
backend/utils/cache_service.py Show resolved Hide resolved
frontend/src/helpers/GetStaticData.js Outdated Show resolved Hide resolved
frontend/src/layouts/page-layout/PageLayout.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

Copy link

sonarcloud bot commented Aug 16, 2024

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.

10 participants