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

fix: Incompatible __client_uat & __session should show interstitial #51

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Mar 15, 2024

Handle 2 interstitial cases:

  • when there is client_uat and cookie token info (both for dev and production apps)
  • when client_uat has a value that does not match with the cookie token value
    • client_uat = 0 (signed-out) but cookie token has value (signed-in)
    • client_uat does NOT have value (??) but cookie token has value (signed-in)
    • client_uat = 123... (signed-in) but cookie token does NOT havue value (signed-out)

@dimkl dimkl requested a review from agis March 15, 2024 19:25
@dimkl dimkl self-assigned this Mar 15, 2024
Copy link
Contributor

@georgepsarakis georgepsarakis left a comment

Choose a reason for hiding this comment

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

👏

if production? && client_uat.nil?
return signed_out(env)
# Show interstitial when there is no client_uat or cookie token
if (client_uat == "0" || client_uat.nil?) && cookie_token.to_s.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ That's the signed-out client_uat value, right? Just noting in case we need to update the comment accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the client_uat == "0" refers to the signed-out state. I updated the code and the comment since it was incorrect to use that. The case here is to render interstitial when there is no client_uat and cookie token information.


# Show interstitial when there is client_uat is incompatible with cookie token
has_cookie_token_without_client = (client_uat == "0" || client_uat.nil?) && cookie_token
has_client_without_cookie_token = (client_uat.to_s != "0" || client_uat.to_s != "") && cookie_token.to_s.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Where does the client_uat.to_s != "" case correspond here to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it corresponds to the case where the client_uat is missing or the value is somehow removed. We could use the client_uat.nil? if that makes it more readable but i think this check is safer because it also applies to cases where the client_uat exists without value (due to some edge-case or extension or something).

return unknown(interstitial: true) if has_cookie_token_without_client || has_client_without_cookie_token

# Show interstitial when there is client_uat is incompatible with cookie token
if (client_uat == "0" || client_uat.nil?) && cookie_token.to_s.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 Same check as in line 174 could be assigned to a variable? Just for aggregating the cases at a single location maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have updated that :)

@dimkl dimkl changed the base branch from main to release/v3 March 19, 2024 13:13
@dimkl dimkl force-pushed the hotfix/interstitial-prod-instance branch from 8dfcca3 to f4954be Compare March 19, 2024 17:05
@dimkl dimkl force-pushed the hotfix/interstitial-prod-instance branch from f4954be to dcad415 Compare March 19, 2024 17:09
@dimkl dimkl force-pushed the hotfix/interstitial-prod-instance branch from dcad415 to 6d8dc34 Compare March 19, 2024 17:19
@dimkl dimkl merged commit 78302a5 into release/v3 Mar 19, 2024
2 checks passed
@dimkl dimkl deleted the hotfix/interstitial-prod-instance branch March 19, 2024 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.

2 participants