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

Alerts for job posts based on filtersets [WIP] #435

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

Conversation

shreyas-satish
Copy link
Contributor

@shreyas-satish shreyas-satish commented Apr 12, 2018

  • Adds the ability to subscribe to alerts for job posts via email. The alerts can be configured for job posts that match one or multiple combinations for filters. Support for push notifications will be added in the next release.
  • Adds a custom __init__ for Filterset to help create filtersets using a dict of filters.

Job alert email
screen shot 2018-05-02 at 11 35 09 am

from premailer import transform as email_transform
from hasjob import mail
from hasjob.models import db, JobPost, JobPostSubscription, JobPostAlert, jobpost_alert_table
from hasjob.views.index import fetch_jobposts
Copy link
Member

Choose a reason for hiding this comment

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

I'm uncomfortable with jobs.* importing from views.*. Jobs are supposed to be independent of views. Can we isolate this (and other associated) functions into a separate helper module? We have many RQ jobs in views/helper.py that need to move as well.

posts = fetch_jobposts(filters=subscription.filterset.to_filters(), posts_only=True)
seen_jobpostids = JobPost.query.join(jobpost_alert_table).join(JobPostAlert).filter(
JobPostAlert.jobpost_subscription == subscription).options(db.load_only('id')).all()
return [post for post in posts if post.id not in seen_jobpostids]
Copy link
Member

Choose a reason for hiding this comment

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

All of this could be a single server-side query.


@job('hasjob')
def send_email_alerts():
for subscription in JobPostSubscription.get_active_subscriptions():
Copy link
Member

Choose a reason for hiding this comment

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

Active+right time must be a single query. You will be skipping more than sending, so this is going to pull in a lot of irrelevant subscriptions.

mail.send(msg)
jobpost_alert.register_delivery()
except Exception as exc:
jobpost_alert.register_failure(unicode(exc))
Copy link
Member

Choose a reason for hiding this comment

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

Since exceptions should not happen, don't do a catch-all. Let this trigger our exception reporting.

@@ -52,6 +53,8 @@ class Filterset(BaseScopedNameMixin, db.Model):

#: Welcome text
description = db.Column(db.UnicodeText, nullable=False, default=u'')
#: Display on sitemap
sitemap = db.Column(db.Boolean, default=False, nullable=True, index=True)
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to allow nullable names and titles as well, and maybe just use that as the sitemap filter.

Copy link
Member

Choose a reason for hiding this comment

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

It just occurred to me that Filterset should use UUID keys, and should use url_name_suuid in URLs, since we'll keep tweaking name and title for SEO.

@@ -90,38 +90,38 @@ def json_index(data):
return jsonify(result)


def fetch_jobposts(request_args, request_values, filters, is_index, board, board_jobs, gkiosk, basequery, md5sum, domain, location, title, showall, statusfilter, batched, ageless, template_vars, search_query=None, query_string=None):
def fetch_jobposts(request_args={}, request_values={}, filters={}, is_index=False, board=None, board_jobs={}, gkiosk=False, basequery=None, md5sum=None, domain=None, location=None, title=None, showall=True, statusfilter=None, batched=True, ageless=False, template_vars={}, search_query=None, query_string=None, posts_only=False):
Copy link
Member

Choose a reason for hiding this comment

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

I think it's time to fix this mess. :-)



@app.route('/subscribe_to_job_alerts', subdomain='<subdomain>', methods=['POST'])
@app.route('/subscribe_to_job_alerts', methods=['POST'])
Copy link
Member

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 should pollute the top-level namespace with these URLs. Park them in a path. /api/1/ should work. See #145 for rationale.

@app.route('/subscribe_to_job_alerts', methods=['POST'])
def subscribe_to_job_alerts():
if not request.json or not request.json.get('filters'):
abort(400)
Copy link
Member

Choose a reason for hiding this comment

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

This should be a requestargs decorator. It doesn't process from request.json, but that's trivial to add in Coaster.

manage.py Outdated
@@ -36,6 +37,12 @@ def campaignviews():
views.helper.reset_campaign_views()


@periodic.command
def send_job_alerts():
Copy link
Member

Choose a reason for hiding this comment

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

Make this send_jobpost_alerts since job and jobpost are distinct concepts.

manage.py Outdated
@periodic.command
def send_job_alerts():
"""Run email alerts every 10 minutes"""
send_email_alerts.delay()
Copy link
Member

Choose a reason for hiding this comment

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

Consider using RQ Scheduler for this: https://github.com/rq/rq-scheduler

@CLAassistant
Copy link

CLAassistant commented May 16, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ vidya-ram
❌ iambibhas
❌ shreyas-satish
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants