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

Asynchronous Notification #2388

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

Conversation

HanilJain
Copy link
Contributor

Asynchronous Notification

Issue Addressed

#2213
#2214
#2215
#2216
#2217
#2218
#2221

How it works

Notification is created in model of notification_app
new_home frontend fetch api Notification.
websockets in header.html file handles the notification coming
memorychannellayer handles the cache and channel serving the notification to webscoket at port 6379
when user click on notification it gets deleted from database.

image

Copy link

sentry-io bot commented Jul 1, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: website/views.py

Function Unhandled Issue
newhome AttributeError: 'AnonymousUser' object has no attribute 'email' ...
Event Count: 3

Did you find this useful? React with a 👍 or 👎

Copy link
Collaborator

@DonnieBLT DonnieBLT left a comment

Choose a reason for hiding this comment

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

Can you please move the code into the existing Django app?

@HanilJain
Copy link
Contributor Author

Can you please move the code into the existing Django app?

@DonnieBLT Done

@HanilJain HanilJain requested a review from DonnieBLT July 1, 2024 14:26
@HanilJain HanilJain marked this pull request as ready for review July 1, 2024 14:29
Copy link
Collaborator

@DonnieBLT DonnieBLT left a comment

Choose a reason for hiding this comment

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

Can you please send the link to the Python anywhere server so that I can test this on a live web server?

blt/settings.py Outdated Show resolved Hide resolved
Copy link
Contributor

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Some comments to address:

blt/settings.py Outdated Show resolved Hide resolved
website/api/views.py Outdated Show resolved Hide resolved
website/api/views.py Outdated Show resolved Hide resolved
website/fixtures/initial_data.json Outdated Show resolved Hide resolved
website/models.py Show resolved Hide resolved
website/views.py Outdated Show resolved Hide resolved
@DonnieBLT
Copy link
Collaborator

Thanks, please make sure those keys are revoked.

Copy link
Contributor

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Please re-request review when you address the comments and resolve the conflicts

@HanilJain
Copy link
Contributor Author

how is it going with the server setup?

With free plan it causing limit CPU usage which increasing the time to deploy additionally memory is full so new dependencies can be installed. I'll try once today and report to you.

@HanilJain HanilJain requested a review from arkid15r July 16, 2024 02:48
@DonnieBLT
Copy link
Collaborator

Thanks for the update. Do you have a branch that I can look at and try to deploy it as well?

@@ -67,6 +67,7 @@
# Application definition

INSTALLED_APPS = (
"daphne",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this

@@ -317,6 +327,7 @@
"0.0.0.0",
"blt.owasp.org",
"." + DOMAIN_NAME_PREVIOUS,
"blt.onrender.com",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this

website/views.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@DonnieBLT DonnieBLT left a comment

Choose a reason for hiding this comment

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

Hanil, there are some change requests here.

@DonnieBLT
Copy link
Collaborator

@HanilJain how is this going?

@DonnieBLT DonnieBLT added the NIL label Aug 6, 2024
@DonnieBLT
Copy link
Collaborator

@HanilJain how is this going?

@HanilJain
Copy link
Contributor Author

@HanilJain how is this going?

Currently I am working on completing the proposal and soon we'll be deploying it.

@DonnieBLT
Copy link
Collaborator

Could you provide an update on whether we've reduced the deploy size under 500MB and if there's a branch where the dependencies are being refactored out?

@HanilJain
Copy link
Contributor Author

Could you provide an update on whether we've reduced the deploy size under 500MB and if there's a branch where the dependencies are being refactored out?

Sir we'll setup a meet with swapnil sir to complete this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants