Skip to content

CV3-55 make notifications calls run on celery.#27

Open
manzif wants to merge 1 commit intodevelopfrom
story/CV3-55-celery-notification-calls
Open

CV3-55 make notifications calls run on celery.#27
manzif wants to merge 1 commit intodevelopfrom
story/CV3-55-celery-notification-calls

Conversation

@manzif
Copy link

@manzif manzif commented Oct 29, 2019

Description

This pull request makes notifications calls run on celery.

How can This be tested

  • Clone the repo: git clone https://github.com/andela/mrm_push.git
  • Setup project according to Readme.md
  • checkout to branch story/CV3-55-celery-notification-calls
  • Run celery celery worker -A cworker.celery --loglevel=info
  • to get notifications, hit the endpoint http://127.0.0.1:5000/notifications

Type of change

Please select the relevant option

  • Bugfix(a non-breaking change which fixes an issue)
  • New feature(a non-breaking change which adds functionality)
  • Breaking change(fix of a feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have linted my code prior to submission
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes

JIRA

CV3-55

@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch from eea2b5a to 28c654a Compare October 29, 2019 09:24
@manzif manzif added the WIP label Oct 29, 2019
@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch 5 times, most recently from 210250e to bf9b6fe Compare November 1, 2019 09:56
@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch from bf9b6fe to 411dafb Compare November 4, 2019 06:53
@manzif manzif changed the title CV3-55-story(notifications): make notifications calls run on celery. CV3-55 make notifications calls run on celery. Nov 4, 2019
@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch 2 times, most recently from 4eb1f0b to fdc6d01 Compare November 4, 2019 07:55
@manzif manzif requested a review from kodek-sleuth November 4, 2019 07:56
@manzif manzif requested a review from joshuaocero November 5, 2019 09:17
Copy link
Contributor

@joshuaocero joshuaocero left a comment

Choose a reason for hiding this comment

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

@manzif This seems to work as expected however I just have an issue with the way your celery tasks are structured. You have turned the whole send_notification function into a celery task which is wrong for a number of reasons;

  • The send_notification function returns a response and having it executed by a celery worker meaning the return will never be used thus making it garbage.
  • The user will no longer receive a JSON response to upon hitting the endpoint.
  • The user will not get feedback on errors because the entire function has been delayed.

I would recommend delaying only the calls to external APIs and keeping the rest of the code the way it is.

@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch 9 times, most recently from b13ba4e to 158d136 Compare November 15, 2019 08:25
@joshuaocero
Copy link
Contributor

@manzif please move the celery tasks to another file. That should be the only change otherwise it looks good.

@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch 2 times, most recently from 317cdac to 221dd0e Compare November 18, 2019 10:41
  - hange these receive calls to be asynchronous and handled by celery

[Delivers CV3-55]
@manzif manzif force-pushed the story/CV3-55-celery-notification-calls branch from 221dd0e to 53b9ca5 Compare November 18, 2019 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants