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

Feature/sap biden ayesha #222

Merged
merged 17 commits into from
Mar 19, 2021
Merged

Feature/sap biden ayesha #222

merged 17 commits into from
Mar 19, 2021

Conversation

ayeshamk
Copy link
Collaborator

No description provided.

@ayeshamk ayeshamk requested review from aih and leedavidr March 15, 2021 16:54
Copy link
Collaborator

@aih aih left a comment

Choose a reason for hiding this comment

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

Requesting a small change (belongs_to field changed to something more descriptive, like administration) and documentation.

operations = [
migrations.AddField(
model_name='statement',
name='belongs_to',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change 'belongs_to' to 'administration'; it is a more descriptive field name and will help future developers understand what the field means.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I created a branch with this change that you could merge into this branch.


To load Relevant Committee Documents data use the following instructions:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add documentation in SCRAPING.adoc for what data is scraped, how to initiate the data. Also in UPDATES_CELERY.adoc to describe the task you've set up for SAP.

@aih
Copy link
Collaborator

aih commented Mar 16, 2021

One more issue as I try it out: it is confusing to have from common.biden_statements import load_statements and the statements.py load_statements(). When I run python manage.py load_statements, I mistakenly run the wrong one (it seems? though they both look for dumped_statements. Please differentiate the commands so that we can include both in the admin tasks without conflict.

@aih
Copy link
Collaborator

aih commented Mar 16, 2021

See also #225. I encountered an issue with saving, possibly due to using MEDIA_ROOT in the filename/link

from .biden_statements import load_statements


@periodic_task(run_every=(crontab(minute=0, hour=0), name="scrape-biden-statements-once-a-day", ignore_result=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

celery @periodic_task decorators are deprecated
See https://stackoverflow.com/a/56071240/628748

I'll move this to celery.py

@aih aih merged commit 3d4c0f7 into main Mar 19, 2021
@aih aih deleted the feature/SAP_Biden_Ayesha branch March 19, 2021 18:55
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.

None yet

2 participants