Skip to content

LG-11784: deletion request 30 days#10189

Merged
mdiarra3 merged 38 commits intomainfrom
LG-11784-deletion-request-30-days
Apr 1, 2024
Merged

LG-11784: deletion request 30 days#10189
mdiarra3 merged 38 commits intomainfrom
LG-11784-deletion-request-30-days

Conversation

@mdiarra3
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-11784

🛠 Summary of changes

This will allow users that are currently in fraud review an extended time before they will be able to reset their account.

📜 Testing Plan

Users not in pending review should not see any change
Users in fraud review for Idv they should now see a longer time before they can receive their account deletion request fulfilled.

@mdiarra3 mdiarra3 marked this pull request as ready for review March 4, 2024 14:28
@mdiarra3 mdiarra3 requested a review from a team March 4, 2024 14:28
@mdiarra3 mdiarra3 changed the title Lg 11784 deletion request 30 days LG-11784: deletion request 30 days Mar 4, 2024
@aduth
Copy link
Contributor

aduth commented Mar 11, 2024

I noticed another two references to the raw config values. Do these need to be updated?

current_time + IdentityConfig.store.account_reset_wait_period_days,

IdentityConfig.store.account_reset_wait_period_days.days.ago,

@aduth aduth requested a review from a team March 19, 2024 19:39
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Any thoughts on findings identified in #10189 (comment) ?

@mdiarra3
Copy link
Contributor Author

Any thoughts on findings identified in #10189 (comment) ?

Yup, I addressed

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Surfacing relevant comment out of old thread: #10189 (comment)

Copy link
Contributor

@aduth aduth Mar 22, 2024

Choose a reason for hiding this comment

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

We don't have test coverage for the scenario where IdentityConfig.store.account_reset_fraud_user_wait_period_days is nil (the default except in test environment). I think there's an error in this scenario.


def fraud_wait_period_not_met?(arr)
if fraud_wait_period_days.present?
return arr.requested_at > (Time.zone.now - fraud_wait_period_days.days)
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to previous comment, fraud_wait_period_days can be nil.

> nil.days
NoMethodError: undefined method `days' for nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching! ill add spec for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it shouldnt occur since fraud_wait_period_days we check if its present. I added a couple nil specs to ensure that this isn't occurring anywhere.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mdiarra3 mdiarra3 merged commit b441b67 into main Apr 1, 2024
@mdiarra3 mdiarra3 deleted the LG-11784-deletion-request-30-days branch April 1, 2024 13:05
@aduth aduth mentioned this pull request Apr 2, 2024
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