-
Notifications
You must be signed in to change notification settings - Fork 14
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
Adds tracking for returning anonymous users #142 #157
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
- Coverage 31.33% 30.48% -0.85%
==========================================
Files 16 16
Lines 1436 1476 +40
==========================================
Hits 450 450
- Misses 986 1026 +40 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functionality works great! I've tested it locally.
I've found some improvements on the code, let's discuss them 😄
ps. can you assign yourself to the original issue? (this flags that you're working on it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you preferred to implement the changes here directly instead of using #159
Good job on using htmltools::htmlDependency
, but it seemed like you overlooked some of the other aspects of the PR:
LICENSE
file (this needs to be distributed withjs-cookie.min.js
, otherwise we might be in breach of the license)
Optional (but I think they can be included here):
- Use of
shiny::singleton
to prevent injection of repeated<script>
tags - Minor optimization of ID using
shiny::NS
instead ofglue::glue
1) moved js.cookie.min.js to a seperate folder and added LICENSE file 2) Used shiny::singleton() over <script> to load the tags only once at app start. 3) remove glue() near Namespace variable and used NS()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Tested it out again and works like it should.
I left one last optional suggestion. Feel free to accept it or not.
Closes #142
Changes description
cookie_string <- paste0(session$request$HTTP_USER_AGENT, session$request$REMOTE_ADDR, Sys.time()) cookie_value <- digest::digest(cookie_string, algo = "sha256")
Note:
authentication: none
. Under this setting, ShinyProxy assigns a random hash to each session as a makeshift user identifier, which is accessible in Shiny applications via theSys.getenv("SHINYPROXY_USERNAME")
command. Refer Official Documentation. This could be easily resolved by adding a condition ,if (Sys.getenv("SHINYPROXY_USERNAME") == <<random_hash>> ) then set the cookie
. But what if shinyproxy user itself is a Random hash?