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.

Sign up for GitHub

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

Jump to bottom

added is_cachable to requirements #608

Merged
merged 3 commits into from
Aug 26, 2024
Merged

added is_cachable to requirements #608

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
167035b
added is_cachable to requirements
alimaktabi Aug 26, 2024
fbef1f8
fixed errors on referencing variable
alimaktabi Aug 26, 2024
de89062
refactored caching constraints
alimaktabi Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/constraints/abstract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class ConstraintVerification(ABC):
_param_keys = []
app_name = ConstraintApp.GENERAL.value
__response_text = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding a comment explaining the purpose and usage of the is_cachable attribute

This attribute controls whether the constraint verification result should be cached. It might be helpful to provide examples of when it should be set to False, such as for time-sensitive or security-critical verifications.

is_cachable = True

def __init__(self, user_profile) -> None:
self.user_profile = user_profile
Expand Down
1 change: 1 addition & 0 deletions core/constraints/captcha.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
class HasVerifiedCloudflareCaptcha(ConstraintVerification):
_param_keys = []
app_name = ConstraintApp.GENERAL.value
is_cachable = False

def is_observed(self, *args, **kwargs) -> bool:

Expand Down
26 changes: 14 additions & 12 deletions prizetap/validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,20 @@ def check_user_constraints(self, raise_exception=True):
is_verified = constraint.is_observed(
**cdata, from_time=int(self.raffle.start_at.timestamp()), context={"request": self.request}
)
caching_time = 60 * 60 if is_verified else 60
expiration_time = time.time() + caching_time
cache_data = {
"is_verified": is_verified,
"info": info,
"expiration_time": expiration_time,
}
cache.set(
cache_key,
cache_data,
caching_time,
)
if constraint.is_cachable:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (complexity): Consider refactoring the caching logic into a separate function.

The new code introduces additional complexity due to the conditional caching logic based on constraint.is_cachable. This adds cognitive load and can make the code harder to read and maintain. Consider refactoring the caching logic into a separate function to encapsulate the behavior and improve readability. This would reduce duplication and make the main function more straightforward. Here's a suggestion:

def cache_constraint_result(cache_key, is_verified, info):
    caching_time = 60 * 60 if is_verified else 60
    expiration_time = time.time() + caching_time
    cache_data = {
        "is_verified": is_verified,
        "info": info,
        "expiration_time": expiration_time,
    }
    cache.set(cache_key, cache_data, caching_time)
    return cache_data

# In the main function
if constraint.is_cachable:
    cache_data = cache_constraint_result(cache_key, is_verified, info)
else:
    cache_data = {"is_verified": is_verified, "info": info}

if not cache_data.get("is_verified"):
    error_messages[c.title] = constraint.response
result[c.pk] = cache_data

This refactoring should help in making the code cleaner and easier to maintain.

caching_time = 60 * 60 if is_verified else 60
expiration_time = time.time() + caching_time
cache_data = {
"is_verified": is_verified,
"info": info,
"expiration_time": expiration_time,
}
cache.set(
cache_key,
cache_data,
caching_time,
)

if not cache_data.get("is_verified"):
error_messages[c.title] = constraint.response
result[c.pk] = cache_data
Expand Down
26 changes: 14 additions & 12 deletions tokenTap/validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,20 @@ def check_user_permissions(self, raise_exception=True):
token_distribution=self.td,
context={"request": self.request}
)
caching_time = 60 * 60 if is_verified else 60
expiration_time = time.time() + caching_time
cache_data = {
"is_verified": is_verified,
"info": info,
"expiration_time": expiration_time,
}
cache.set(
cache_key,
cache_data,
caching_time,
)
if constraint.is_cachable:
caching_time = 60 * 60 if is_verified else 60
expiration_time = time.time() + caching_time
cache_data = {
"is_verified": is_verified,
"info": info,
"expiration_time": expiration_time,
}

cache.set(
cache_key,
cache_data,
caching_time,
)
if not cache_data.get("is_verified"):
error_messages[c.title] = constraint.response
result[c.pk] = cache_data
Expand Down
Loading