Send notification content through Firebase and add an Arq scheduler by foucblg · Pull Request #613 · aeecleclair/Hyperion

foucblg · 2024-10-29T15:46:56Z

No description provided.

app/core/endpoints_core.py

app/types/scheduler.py

app/utils/communication/future_notifications.py

app/worker.py

codecov · 2024-11-17T18:15:47Z

Codecov Report

Attention: Patch coverage is 53.26087% with 86 lines in your changes missing coverage. Please review.

Project coverage is 80.99%. Comparing base (66e7531) to head (a0ce052).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
app/types/scheduler.py	45.94%	40 Missing ⚠️
app/utils/communication/notifications.py	55.81%	19 Missing ⚠️
app/core/notification/endpoints_notification.py	56.25%	7 Missing ⚠️
app/app.py	14.28%	6 Missing ⚠️
app/modules/ph/endpoints_ph.py	16.66%	5 Missing ⚠️
app/dependencies.py	57.14%	3 Missing ⚠️
app/utils/tools.py	57.14%	3 Missing ⚠️
app/modules/cinema/endpoints_cinema.py	50.00%	1 Missing ⚠️
app/modules/loan/endpoints_loan.py	91.66%	1 Missing ⚠️
app/types/exceptions.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
- Coverage   81.31%   80.99%   -0.33%     
==========================================
  Files         129      130       +1     
  Lines       10013    10116     +103     
==========================================
+ Hits         8142     8193      +51     
- Misses       1871     1923      +52

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

armanddidierjean

We should add a test that tries to defer a method using the scheduler

app/core/endpoints_core.py

app/dependencies.py

app/modules/advert/endpoints_advert.py

app/modules/ph/endpoints_ph.py

app/types/scheduler.py

app/dependencies.py

app/modules/loan/endpoints_loan.py

app/types/scheduler.py

armanddidierjean · 2024-12-02T13:54:35Z

app/types/scheduler.py

+        )
+        scheduler_logger.debug(f"Job {job_id} queued {job}")
+
+    async def queue_job_defer_to(


Is there a real interest in keeping both queue_job_time_defer and queue_job_defer_to?
I think we are likely to calculate defer_seconds using a timedelta and datetime, and thus could easily use queue_job_defer_to instead.

It seems Arq will calculate a datetime in both cases, so there shouldn't be a performance gain from using a method or the other.

I would think only having one method will reduce code complexity and limit potentials mistakes

armanddidierjean

We will improve the remaining points in the future
#647

Co-authored-by: Armand Didierjean <95971503+armanddidierjean@users.noreply.github.com>

Simplifying expression Co-authored-by: Armand Didierjean <95971503+armanddidierjean@users.noreply.github.com>

as the new structure does not require it

instead of default value

> [!IMPORTANT] > We really need to release new versions **soon**, as we have not for several months and there's some time needed to test in alpha before prod. This goes hand in hand with the **major version bump** [for Titan](../../Titan/pull/462). Both PRs are ready, and _seemingly_ waiting for some more **breaking changes** to be merged, including (by likelihood): - [ ] Allow students from other Schools to have a MyECL account: #641 - [ ] MyECLPay: #611 - [ ] "_Centrale Mega Meme_" Meme module: [no PR yet] [see branch](../tree/cmm) - [x] External Notifications: #613

armanddidierjean added enhancement New feature or request notifications labels Nov 16, 2024

foucblg force-pushed the Notification-final-branch branch 3 times, most recently from e9e646b to 07a5ce5 Compare November 17, 2024 04:49

armanddidierjean marked this pull request as draft November 17, 2024 08:06

armanddidierjean requested changes Nov 17, 2024

View reviewed changes

armanddidierjean reviewed Nov 17, 2024

View reviewed changes

app/worker.py Outdated Show resolved Hide resolved

foucblg added the ready for review This PR is ready to be reviewed label Nov 20, 2024

foucblg marked this pull request as ready for review November 25, 2024 12:30

Daihecyy approved these changes Nov 25, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

armanddidierjean requested changes Nov 25, 2024

View reviewed changes

armanddidierjean requested changes Dec 2, 2024

View reviewed changes

app/dependencies.py Outdated Show resolved Hide resolved

app/modules/loan/endpoints_loan.py Outdated Show resolved Hide resolved

app/types/scheduler.py Show resolved Hide resolved

armanddidierjean reviewed Dec 2, 2024

View reviewed changes

armanddidierjean changed the title ~~Notification final branch~~ Send notification content through Firebase and add an Arq scheduler Dec 3, 2024

Daihecyy added wontfix This will not be worked on and removed wontfix This will not be worked on labels Dec 3, 2024

Rotheem force-pushed the Notification-final-branch branch 3 times, most recently from 4cadc2f to 0eb2466 Compare December 4, 2024 19:09

Marc-Andrieu mentioned this pull request Dec 25, 2024

Bump version 4.0.0 #642

Merged

4 tasks

armanddidierjean approved these changes Jan 2, 2025

View reviewed changes

Daihecyy added 6 commits January 2, 2025 20:35

First test for scheduler

b6991bd

Fix docker files

ad97e65

Worker redis settings configuration

bd61a1a

Fixed stupid mistake

e25aa96

Fix depency scheduler

bad5a7e

Denesting test function

6e80232

foucblg and others added 26 commits January 2, 2025 20:35

Update app/modules/advert/endpoints_advert.py

40cef5c

Co-authored-by: Armand Didierjean <95971503+armanddidierjean@users.noreply.github.com>

Update app/types/scheduler.py

c7d5f1c

Simplifying expression Co-authored-by: Armand Didierjean <95971503+armanddidierjean@users.noreply.github.com>

Defer seconds -> int

1dbc14d

Feat : unique function to send notif

949573a

Fix avoiding n notification for cine

3aebcc4

Fix : update topic membership for new tokens

de853f8

lint and format grmf

11c9482

Revert test line

03ce388

Fix : Cancel notification with notif manager

205e0d7

Dummy Scheduler to Offlinescheduler

c6b36fa

Feat : removing scheduler time defer

69dd104

Feat : arq logging

716def3

Fix : arq.warker log

0c61eec

Dependency injection in the scheduler

0c641a6

Lint and format

ba99ad5

Fix: rebase

7eea068

Ruff

89eea29

Inject db in run_task

4ceea58

Don't pass db=None

797932d

as the new structure does not require it

Add test endpoint

7f4f548

Fix action module

75fc340

Fix action module

f992303

Fix import

d9611c2

Check if annotation is AsyncSession

556f244

instead of default value

Fix missing action_module

cabe342

Fixing test

6b40657

armanddidierjean force-pushed the Notification-final-branch branch from a0ce052 to 6b40657 Compare January 2, 2025 19:36

armanddidierjean merged commit 82febab into main Jan 2, 2025

armanddidierjean deleted the Notification-final-branch branch January 2, 2025 19:44

Conversation

foucblg commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as resolved.

Uh oh!

armanddidierjean left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

armanddidierjean Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

armanddidierjean left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 17, 2024 •

edited

Loading