Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Gifted Stickers for incomplete users #437

Merged
merged 55 commits into from
Nov 18, 2019

Conversation

fridaland
Copy link
Contributor

@fridaland fridaland commented Nov 13, 2019

Distributing remaining sticker coupons

A new state will be added to the user state machine (gifted_sticker), to distribute all remaining sticker coupons. This will include:

  • Validating the user upon transitioning from incomplete -> gifted_sticker
  • Appropriate Segment calls to fire email events
  • Background job to transition users based off of how many scoring PRs they have. We want to transition based on:
    • Highest # of PRs
    • Ordered by earliest created_at time of the latest pull request

Test process

  • Test new gifted_sticker state
  • Test validation for coupon presence for new state in even machine
  • Test transition for a user in the incomplete state to gifted_sticker
  • Test service GiftStickersService that will assign user a sticker coupon

Open Source contributing guidelines

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@johndbritton
Copy link
Contributor

Ordered by earliest created_at time

This should be more specific. Ordered by earliest created_at time of the latest pull request.

u.scoring_pull_requests.max_by(&:created_at).created_at

app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
@jhaff
Copy link
Contributor

jhaff commented Nov 14, 2019

Of note: it's odd that we did not have to stub the UserStateTransitionSegmentService in the gift_stickers_service_spec because ultimately, the GiftStickerService is causing transitions, which should cause the UserStateTransitionSegmentService to be called

Copy link
Contributor

@johndbritton johndbritton left a comment

Choose a reason for hiding this comment

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

Looking good. Few things to be renamed and some minor fixes. Probably will need one or two more rounds.

app/services/coupon_service.rb Show resolved Hide resolved
app/services/gift_stickers_service.rb Outdated Show resolved Hide resolved
app/services/gift_stickers_service.rb Outdated Show resolved Hide resolved
app/services/gift_stickers_service.rb Outdated Show resolved Hide resolved
lib/tasks/stickers.rake Outdated Show resolved Hide resolved
spec/services/coupon_service_spec.rb Outdated Show resolved Hide resolved
spec/services/coupon_service_spec.rb Show resolved Hide resolved
spec/services/coupon_service_spec.rb Outdated Show resolved Hide resolved
spec/services/gift_sticker_service_spec.rb Outdated Show resolved Hide resolved
Comment on lines 22 to 29
user_with_early_receipt_2prs
.update(receipt: UserReceiptHelper.receipt[:early_receipt_2_prs])
user_with_early_receipt_3prs
.update(receipt: UserReceiptHelper.receipt[:early_receipt_3_prs])
user_with_late_receipt_2prs
.update(receipt: UserReceiptHelper.receipt[:late_receipt_2_prs])
user_with_late_receipt_3prs
.update(receipt: UserReceiptHelper.receipt[:late_receipt_3_prs])
Copy link
Contributor

Choose a reason for hiding this comment

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

This starter data should be updated to reflect the logic that a user with the earliest date on their latest PR should be ranked first for users with that number of PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually already the case. My main mistake was just grabbing the first created PR per user as opposed to the last in the GiftService . However, the user with the earliest date on their latest PR will always be ranked first. So if there is only one coupon user_with_early_receipt_3prs (meaning user with score of 3, who's last PR has earliest PR created_at time) will be the one getting the coupon.

fridaland and others added 9 commits November 14, 2019 13:47
Co-Authored-By: John Britton <public@johndbritton.com>
Co-Authored-By: John Britton <public@johndbritton.com>
Co-Authored-By: John Britton <public@johndbritton.com>
Co-Authored-By: John Britton <public@johndbritton.com>
Co-Authored-By: John Britton <public@johndbritton.com>
@fridaland fridaland marked this pull request as ready for review November 14, 2019 20:44
@fridaland fridaland mentioned this pull request Nov 14, 2019
10 tasks
@fridaland
Copy link
Contributor Author

fridaland commented Nov 14, 2019

Screen Shot 2019-11-14 at 6 26 14 PM
Since we can't run CI

app/services/gift_service.rb Outdated Show resolved Hide resolved
app/services/gift_service.rb Outdated Show resolved Hide resolved
app/services/gift_service.rb Outdated Show resolved Hide resolved
app/services/gift_service.rb Show resolved Hide resolved
Copy link
Contributor

@johndbritton johndbritton 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 couple questions

spec/models/user_spec.rb Show resolved Hide resolved
spec/models/user_spec.rb Outdated Show resolved Hide resolved
spec/models/user_spec.rb Show resolved Hide resolved
app/services/gift_service.rb Show resolved Hide resolved
fridaland and others added 2 commits November 18, 2019 15:12
Co-Authored-By: John Britton <public@johndbritton.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants