Enable FullStory for users on the PERSONAL30 plan #13753

alonsogarciapablo · 2018-03-23T15:43:38Z

I was missing some sessions in FullStory. It turns out it was enabled for everyone on the signup flow (commercial site), but only enabled for users on the FREE plan in Dashboard / Builder.

This PR introduces some tweaks to the FullStory setup:

FS will only be enabled for users who signed up after 2017-01-01.
FS will be enabled for FREE and PERSONAL30 users, so that we can record the first sessions of users.
FS will be enabled in the Dataset view but the contents of the cells will be excluded from the recordings, as they might contain sensitive data (https://help.fullstory.com/technical-questions/exclude-elements).
FS will also be tracking accountType in Dashboard / Builder / Dataset view.

There has been some discussion about this in https://github.com/CartoDB/cartodb-management/issues/5020.

cc: @piensaenpixel @arianaescobar

houndci-bot · 2018-03-23T15:43:49Z

app/helpers/application_helper.rb

@@ -119,7 +119,7 @@ def insert_hubspot_form(form = 'newsletter')

  def insert_fullstory
    if Cartodb.get_config(:fullstory, 'org').present? && current_user &&
-       current_user.account_type.casecmp('FREE').zero? && params[:cookies] != '0'
+      ['FREE','PERSONAL30'].include?(current_user.account_type.upcase) && params[:cookies] != '0'


Align the operands of a condition in an if statement spanning multiple lines.
Space missing after comma.

arianaescobar · 2018-03-23T16:46:52Z

Wait wait wait, we can't do this.

arianaescobar · 2018-03-23T16:49:32Z

@alonsogarciapablo take a look at this: https://github.com/CartoDB/cartodb-management/issues/5020

alonsogarciapablo · 2018-03-26T08:58:16Z

Thanks @arianaescobar! I will take a look and make sure this won't put us all in jail! 😂

…nal30

- Make sure that FullStory is only enabled for users who accepted the Terms of Service and Privacy Polity during the signup process. - Enable FullStory in the Dataset page, filtering out sensitive data.

houndci-bot · 2018-03-28T13:00:59Z

app/helpers/application_helper.rb

+    if fullstory_org.present? &&
+      user_plan_supported &&
+      user_signed_up_after_min_signed_up_date &&
+      params[:cookies] != '0'


Align the operands of a condition in an if statement spanning multiple lines.

houndci-bot · 2018-03-28T13:01:00Z

app/helpers/application_helper.rb

+      current_user.created_at > min_signed_up_date
+
+    if fullstory_org.present? &&
+      user_plan_supported &&


Align the operands of a condition in an if statement spanning multiple lines.

houndci-bot · 2018-03-28T13:01:00Z

app/helpers/application_helper.rb

+    user_plan_supported = current_user &&
+      ['FREE', 'PERSONAL30'].include?(current_user.account_type.upcase)
+    user_signed_up_after_min_signed_up_date = current_user &&
+      current_user.created_at > min_signed_up_date


Align the operands of an expression in an assignment spanning multiple lines.

houndci-bot · 2018-03-28T13:01:00Z

app/helpers/application_helper.rb

+    min_signed_up_date = Date.new(2017,01,01)
+    fullstory_org = Cartodb.get_config(:fullstory, 'org')
+    user_plan_supported = current_user &&
+      ['FREE', 'PERSONAL30'].include?(current_user.account_type.upcase)


Align the operands of an expression in an assignment spanning multiple lines.

houndci-bot · 2018-03-28T13:01:00Z

app/helpers/application_helper.rb

+    # which is the date where a message to accept the Terms and
+    # conditions and the Privacy policy was included in the Signup page.
+    # See https://github.com/CartoDB/cartodb-central/commit/3627da19f071c8fdd1604ddc03fb21ab8a6dff9f
+    min_signed_up_date = Date.new(2017,01,01)


Space missing after comma.
Use 0o for octal literals.

alonsogarciapablo · 2018-03-28T14:08:48Z

Talked to @agnesgyorfi about this and made some changes to the PR. I updated the PR description and explained there how it works now.

…nal30

houndci-bot · 2018-03-28T15:24:49Z

app/helpers/application_helper.rb

+
+    if fullstory_org.present? &&
+      user_plan_supported &&
+      user_signed_up_after_min_signed_up_date &&


Align the operands of a condition in an if statement spanning multiple lines.

javitonino

Please, apply Hound suggestions. Also, consider:

We have 120 column lines in Ruby. Use them! ;)
You could do a return unless current_user at the beginning of the method and avoid a couple of current_user checks.

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/DefEndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/BlockAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/ConditionPosition has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)

…nal30

houndci-bot · 2018-04-09T10:35:35Z

app/helpers/application_helper.rb

+    # which is the date where a message to accept the Terms and
+    # conditions and the Privacy policy was included in the Signup page.
+    # See https://github.com/CartoDB/cartodb-central/commit/3627da19f071c8fdd1604ddc03fb21ab8a6dff9f
+    min_signed_up_date = Date.new(2017, 01, 01)


Use 0o for octal literals.

alonsogarciapablo · 2018-04-09T10:40:20Z

Thanks for your feedback @javitonino! I changed the code a bit in a892a81.

alonsogarciapablo · 2018-04-10T10:20:59Z

app/helpers/application_helper.rb

+    min_signed_up_date = Date.new(2017, 01, 01)
+    fullstory_org = Cartodb.get_config(:fullstory, 'org')
+    user_plan_supported = ['FREE', 'PERSONAL30'].include?(current_user.account_type.upcase)
+    user_signed_up_after_min_signed_up_date = current_user.created_at > min_signed_up_date


@javitonino I noticed a similar logic exists here. It looks like in other places we use the fullstoryEnabled variable to determine if FullStory is enabled. Basically: here and here. I have no idea what those files are being used for, but it seems that we need to DRY this up a bit.

houndci-bot · 2018-04-10T11:14:04Z

app/models/user.rb

+  # which is the date where a message to accept the Terms and
+  # conditions and the Privacy policy was included in the Signup page.
+  # See https://github.com/CartoDB/cartodb-central/commit/3627da19f071c8fdd1604ddc03fb21ab8a6dff9f
+  FULLSTORY_ENABLED_MIN_DATE = Date.new(2017, 01, 01)


Use 0o for octal literals.

houndci-bot · 2018-04-10T11:14:04Z

app/models/carto/user.rb

+  # which is the date where a message to accept the Terms and
+  # conditions and the Privacy policy was included in the Signup page.
+  # See https://github.com/CartoDB/cartodb-central/commit/3627da19f071c8fdd1604ddc03fb21ab8a6dff9f
+  FULLSTORY_ENABLED_MIN_DATE = Date.new(2017, 01, 01)


Use 0o for octal literals.

houndci-bot · 2018-04-10T11:14:04Z

app/helpers/frontend_config_helper.rb

@@ -6,6 +6,7 @@ module FrontendConfigHelper
  UPGRADE_LINK_ACCOUNT = 'PERSONAL30'.freeze

  def frontend_config_hash(user = current_user)
+    byebug


Remove debugger entry point byebug.

alonsogarciapablo · 2018-04-10T11:22:17Z

app/models/carto/user.rb

@@ -643,6 +650,10 @@ def locked?
    state == STATE_LOCKED
  end

+  def fullstory_enabled?


I thought about moving this logic to the user too. Love it. 👏👏👏

Perhaps worth extracting this logic to a module (concern?) that both Carto::User and User can share? There's the risk of someone changing this logic in only one place otherwise...

No need. This is the standard approach for duplicated Sequel/AR models, we just copy funcionality. We are always aware we need to change it at both places. And it's just another reason to finally deprecate the Sequel model :D

alonsogarciapablo · 2018-04-10T11:24:06Z

app/helpers/fullstory_helper.rb

@@ -0,0 +1,11 @@
+module FullstoryHelper
+  def fullstory_enabled?(user)


I personally think this helper would read better if we inverse the order of these two methods.

alonsogarciapablo · 2018-04-10T11:53:17Z

@agnesgyorfi FYI: this is how the Dataset view looks like in FS recordings (content of cells are greyed out):

…nal30

Done

…/cartodb into fullstory-for-personal30

alonsogarciapablo · 2018-04-10T15:22:29Z

Deployin' 🚀

Enable FullStory for users on the PERSONAL30 plan

6721370

houndci-bot reviewed Mar 23, 2018

View reviewed changes

alonsogarciapablo requested a review from javitonino March 23, 2018 15:44

Pablo Alonso Garcia added 2 commits March 23, 2018 16:49

NEWS

1d32d05

whitespace

06ee860

Pablo Alonso Garcia added 2 commits March 27, 2018 15:58

Merge remote-tracking branch 'origin/master' into fullstory-for-perso…

fc6e8a2

…nal30

FullStory

c6b9806

- Make sure that FullStory is only enabled for users who accepted the Terms of Service and Privacy Polity during the signup process. - Enable FullStory in the Dataset page, filtering out sensitive data.

houndci-bot reviewed Mar 28, 2018

View reviewed changes

Whitespace

619d1d6

alonsogarciapablo removed the request for review from javitonino March 28, 2018 13:29

Pablo Alonso Garcia added 3 commits March 28, 2018 15:30

NEWS.md

6760fd5

Exclude cell's paragraph from FullStory

e6642dc

Send accountType to FullStory

153c017

CartoDB deleted a comment from houndci-bot Mar 28, 2018

Merge remote-tracking branch 'origin/master' into fullstory-for-perso…

3809591

…nal30

houndci-bot reviewed Mar 28, 2018

View reviewed changes

javitonino previously requested changes Apr 2, 2018

View reviewed changes

Code style

a892a81

houndci-bot reviewed Apr 9, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into fullstory-for-perso…

501ca41

…nal30

houndci-bot reviewed Apr 9, 2018

View reviewed changes

alonsogarciapablo commented Apr 10, 2018

View reviewed changes

javitonino self-assigned this Apr 10, 2018

Javier Torres added 2 commits April 10, 2018 12:57

Merge branch 'master' into fullstory-for-personal30

7700e92

Extract fullstory helpers

19fcc20

Proper modulization

d839190

houndci-bot reviewed Apr 10, 2018

View reviewed changes

Hound

3a70797

alonsogarciapablo commented Apr 10, 2018

View reviewed changes

CR: reorder helper methods

67a87da

Merge remote-tracking branch 'origin/master' into fullstory-for-perso…

1a8b306

…nal30

Javier Torres and others added 3 commits April 10, 2018 14:31

Include fullstory helper

3ef9488

Merge branch 'fullstory-for-personal30' of https://github.com/CartoDB…

04eac3f

…/cartodb into fullstory-for-personal30

Fix custom data variable

938c2d9

alonsogarciapablo merged commit f8b8923 into master Apr 10, 2018

alonsogarciapablo deleted the fullstory-for-personal30 branch April 10, 2018 15:21

alonsogarciapablo unassigned javitonino Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable FullStory for users on the PERSONAL30 plan #13753

Enable FullStory for users on the PERSONAL30 plan #13753

alonsogarciapablo commented Mar 23, 2018 •

edited

Loading

houndci-bot Mar 23, 2018

arianaescobar commented Mar 23, 2018

arianaescobar commented Mar 23, 2018

alonsogarciapablo commented Mar 26, 2018

houndci-bot Mar 28, 2018

houndci-bot Mar 28, 2018

houndci-bot Mar 28, 2018

houndci-bot Mar 28, 2018

houndci-bot Mar 28, 2018

alonsogarciapablo commented Mar 28, 2018

houndci-bot Mar 28, 2018

javitonino left a comment

houndci-bot left a comment

houndci-bot Apr 9, 2018

alonsogarciapablo commented Apr 9, 2018

alonsogarciapablo Apr 10, 2018

houndci-bot Apr 10, 2018

houndci-bot Apr 10, 2018

houndci-bot Apr 10, 2018

alonsogarciapablo Apr 10, 2018

alonsogarciapablo Apr 10, 2018 •

edited

Loading

javitonino Apr 10, 2018

alonsogarciapablo Apr 10, 2018

alonsogarciapablo commented Apr 10, 2018

alonsogarciapablo commented Apr 10, 2018

		@@ -0,0 +1,11 @@
		module FullstoryHelper
		def fullstory_enabled?(user)

Enable FullStory for users on the PERSONAL30 plan #13753

Enable FullStory for users on the PERSONAL30 plan #13753

Conversation

alonsogarciapablo commented Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

arianaescobar commented Mar 23, 2018

arianaescobar commented Mar 23, 2018

alonsogarciapablo commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonsogarciapablo commented Mar 28, 2018

Choose a reason for hiding this comment

javitonino left a comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonsogarciapablo commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonsogarciapablo Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonsogarciapablo commented Apr 10, 2018

alonsogarciapablo commented Apr 10, 2018

alonsogarciapablo commented Mar 23, 2018 •

edited

Loading

alonsogarciapablo Apr 10, 2018 •

edited

Loading