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

Perk Fulfillment #491

Merged
merged 46 commits into from
Jan 5, 2023
Merged

Perk Fulfillment #491

merged 46 commits into from
Jan 5, 2023

Conversation

cassidysymons
Copy link
Collaborator

@cassidysymons cassidysymons commented Nov 23, 2022

No description provided.

Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

About 40% through, issuing this review right now as I'm out of time for the day

microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/email_templates.py Outdated Show resolved Hide resolved
microsetta_private_api/db/patches/0108.sql Outdated Show resolved Hide resolved
microsetta_private_api/db/patches/0108.sql Outdated Show resolved Hide resolved
microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
@wasade
Copy link
Member

wasade commented Nov 23, 2022 via email

@cassidysymons
Copy link
Collaborator Author

@wasade I believe I've either resolved or responded to each comment you left on Wednesday. If I resolved a comment, it means I have updated code ready to push up to address it. Would you prefer I push those updates before you resume the code review? Or do you want to finish looking through the code you haven't reviewed before I push updates?

@wasade
Copy link
Member

wasade commented Nov 28, 2022

@cassidysymons, thanks for checking. I think push now, and I'll re-review -- what I've already gone through will be quick

Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

Please make sure to render the template emails. There are many instances where quote marks cancel out, and less important but tentatively problematic are the use of non-standard quote marks.

microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
cur_date = datetime.now()
if spacing_unit == "days":
new_date = cur_date + relativedelta(
days=+(spacing_number*multiplier)
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the +? It doesn't ensure the value is positive:

>>> +(-1)
-1
>>>

If that is the intention, I recommend abs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The + is following the recommended usage of the relative keyword arguments from the dateutil docs. That said, adding an abs() around the spacing_number variable is a good call to ensure all scheduling is in the future.

Copy link
Member

Choose a reason for hiding this comment

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

So weird, it seems like it's a noop?

>>> +1
1
>>> +(-1)
-1
>>>

microsetta_private_api/util/fundrazr.py Outdated Show resolved Hide resolved
Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

Just a few what I think are small items and questions

microsetta_private_api/celery_utils.py Outdated Show resolved Hide resolved
microsetta_private_api/celery_worker.py Show resolved Hide resolved
microsetta_private_api/repo/perk_fulfillment_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/model/subscription.py Outdated Show resolved Hide resolved
@cassidysymons cassidysymons merged commit 06344e9 into master-overhaul Jan 5, 2023
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.

2 participants