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.

Sign up for GitHub

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

Jump to bottom

Asynchronous Notification #2388

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

Asynchronous Notification #2388

wants to merge 21 commits into from

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

@sentry-io Sentry.io
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 👎

DonnieBLT
DonnieBLT requested changes Jul 1, 2024
View reviewed changes
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
@HanilJain HanilJain mentioned this pull request Jul 2, 2024
Open
DonnieBLT
DonnieBLT requested changes Jul 2, 2024
View reviewed changes
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?

DonnieBLT
DonnieBLT requested changes Jul 2, 2024
View reviewed changes
blt/settings.py Outdated Show resolved Hide resolved
arkid15r
arkid15r requested changes Jul 3, 2024
View reviewed changes
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
HanilJain and others added 2 commits July 4, 2024 02:08
@HanilJain @arkid15r
Apply suggestions from code review
df73ca7 
suggested changes

Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
@HanilJain
Merge branch 'main' into async_project
4920eff
@DonnieBLT
Copy link
Collaborator

Thanks, please make sure those keys are revoked.

arkid15r
arkid15r reviewed Jul 11, 2024
View reviewed changes
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

@DonnieBLT
Copy link
Collaborator

how is it going with the server setup?

@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?

DonnieBLT
DonnieBLT reviewed Jul 21, 2024
View reviewed changes
blt/settings.py
@@ -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

DonnieBLT
DonnieBLT reviewed Jul 21, 2024
View reviewed changes
blt/settings.py
@@ -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

DonnieBLT
DonnieBLT reviewed Jul 21, 2024
View reviewed changes
website/views.py
@@ -280,11 +281,35 @@ def newhome(request, template="new_home.html"):
context = {
"bugs": page_obj,
"bugs_screenshots": bugs_screenshots,
"leaderboard": User.objects.filter(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we already have leaderboard

DonnieBLT
DonnieBLT requested changes Jul 21, 2024
View reviewed changes
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
Reviewers

@DonnieBLT DonnieBLT DonnieBLT requested changes

@arkid15r arkid15r Awaiting requested review from arkid15r

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
NIL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HanilJain @DonnieBLT @arkid15r