Delete users in segment when they are deleted from system #717

Martouta · 2019-03-21T11:20:13Z

Closes THREESCALE-1846 - Delete users in segment when they are deleted from system
We will need https://github.com/3scale/deploy/pull/550 merged for it to work, but without it, it simply does nothing 😄

Tasks of this PR

Add a SegmentDeletionConfig as a config feature containing : enabled, email, password, uri, workspace.
When a User is destroyed, save a simple UserDeletedEvent. The only data we need is the user ID and the tenant ID.
Segment::AuthenticatorService to request token. It uses Faraday and raises an error for wrong responses.
Segment::DeleteUserService to delete the user from the user_id it receives and with the token sent as param.
When the UserDeletedEvent is created, it is destroyed in segment in the background
When segment deletion feature is disabled, do not even enqueue the worker
Remove all the code of the old SegmentSubscriber.
Remove programatically the first_admin_id of Account.

Why to remove the SegmentSubscriber

the SegmentSubscriber to call segment for the accounts that were deleted to mark their first admin as 'state: deleted' in segment. It was by account and we want it for user, and it didn't delete it for real them for real but just put that state attribute on them.

Why to remove the first_admin_id

Some of the times this explanation has been requested are: #717 (comment) & #717 (comment) & #717 (comment)

The first_admin_id was introduced in #8 to fix this bug https://github.com/3scale/porta/issues/10 . It was introduced 5 months ago. The problem was that AccountDeletedEvent was saving the user_id of the first admin but at the time this event was being saved, the user had already been deleted and we didn't have the ID anymore. And we needed it only for the SegmentSubscriber. Now that we have removed the SegmentSubscriber, we don't need the first_admin_id at all anymore. Although that time has changed it is not used anywhere else for anything:

In preview only seen that the configuration is correct

I checked in preview that the configuration of Features::SegmentDeletionConfig is correct for https://github.com/3scale/deploy/pull/550, and then I am testing locally in development environment with that same configuration... with the production one though, and using the list of users that need to be destroyed for real...
These are the reasons:

Apparently we don't always do the tracking thing when we create new providers, which is probably a bug. When it goes through some specific controllers and ProviderAccountManager, i think it doesn't happen. Anyway, if this is a bug, it is not a bug problem for this issue by itself, we still need to figure it out because if it is a bug, we need to create a jira issue for this, but we can bypass this problem for this testing because we can execute the Account.master.third_party_notifications!(tenant) for this method.
Even bypassing the previous one, i don't see anything created in segment because:
2.1 We are not specifying this workspace and url for preview anywhere, so it is impossible that it goes there to create the users
2.2 In segment we can not see the users itself, we need integration with intercom, and apparently there is an intercom for testing but i couldn't access it...

Verification steps (locally and with real production data, so be careful with this!)

Open a terminal and checkout to this Git branch.
Edit the file config/features.yml to have a config that can access to the production data. You can see the proper format in config/examples/features.yml and the proper data in https://github.com/3scale/deploy/pull/550

Edit the following method to return true always, so you can delete any user.

porta/app/models/user.rb

Lines 416 to 419 in 3f7fa1d

    
           def destroyable? 
        
             return true if destroyed_by_association 
        
             !(account && !account.destroyed? && account.admins == [self]) 
        
           end

Optional step, but personally I reseted the whole local DB and I have done redis-cli flushall to have all the sidekiq queue clean.
Either create a user or use one of the users that you already have in the DB, either way got to the rails console and do User.last.update_column(:id, user_id) with the user_id of one of those that have to be destroyed for real and has not been destroyed yet.
Close the rails console, stop spring if you have it running (spring stop). Turn on the Rails Server: OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES UNICORN_WORKERS=8 bundle exec rails server -b 0.0.0.0
In another tab of the terminal, turn on Sidekiq with the default queue: UNICORN_WORKERS=8 bundle exec sidekiq -q low -q default -L log/sidekiq.log
In another tab of the terminal, check the logs of Sidekiq: tail -f log/sidekiq.log
In the UI, go to the user edit page and delete it, in many case it was in http://provider-admin.example.com.lvh.me:3000/buyers/accounts/3/users/:user_id/edit.
You should see in the logs of Sidekiq something like this:

2019-05-06T15:00:00.051Z 19691 TID-ox1a4veen SegmentDeleteUserWorker JID-1afb023df04f1c4f4c013d55 INFO: start
...
2019-05-06T15:00:01.827Z 19691 TID-ox1a4veen SegmentDeleteUserWorker JID-1afb023df04f1c4f4c013d55 INFO: done: 1.776 sec

Go the deletion requests page in the Segment page, and see that the user with this ID is already there. You will see that it says FAILED or INITIALIZED, which means that it works.
The FAILED means that segment does a request to the 3rd party services and the 3rd party services might fail and we can not do anything about it, and for this PR this is still just fine, so for this PR, FAILED would mean it worked, because we got to notify segment, and we would have a problem only if it doesn't appear at all in the deletion requests page in the Segment page.

thomasmaas · 2019-03-21T11:23:30Z

app/events/user_related_event.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class UserRelatedEvent < BaseEventStoreEvent


@Martouta I assume this is to prepare for other user related events that might be added in the future? Not premature?

Looking at user_related_event_test.rbI guess it's just the way we structure events

Yes, it is the way that structure events, and also the way that we structure Subscribers to those events (grouping by 'related' events 😄 )

user related event makes sense.

test/events/users/user_deleted_event_test.rb

+    assert_equal event.metadata[:provider_id], master_account.id
+  end
+
+  test 'the event is persisted and with the necessary attributes when its associations are already destroyed' do


test/unit/user_test.rb

@@ -20,6 +20,16 @@ class UserTest < ActiveSupport::TestCase
    ActionMailer::Base.deliveries = []
  end

+  test 'the event is created when the user is destroyed' do


thomasmaas

left some comments and suggestions to improve test decriptions

codecov · 2019-03-21T15:39:26Z

Codecov Report

Merging #717 into master will increase coverage by 0.94%.
The diff coverage is 99.65%.

@@            Coverage Diff             @@
##           master     #717      +/-   ##
==========================================
+ Coverage      92%   92.94%   +0.94%     
==========================================
  Files        2321     2413      +92     
  Lines       74204    78298    +4094     
==========================================
+ Hits        68269    72777    +4508     
+ Misses       5935     5521     -414

Impacted Files	Coverage Δ
test/unit/signup/result_test.rb	`100% <ø> (ø)`	⬆️
app/events/accounts/account_deleted_event.rb	`100% <ø> (ø)`	⬆️
test/unit/logic/provider_signup_test.rb	`98.59% <ø> (ø)`
app/lib/logic/provider_signup.rb	`99.18% <ø> (+34.95%)`	⬆️
test/unit/user_test.rb	`98.84% <100%> (+0.01%)`	⬆️
app/lib/features/config.rb	`100% <100%> (ø)`
app/events/users/user_deleted_event.rb	`100% <100%> (ø)`
.../unit/services/segment/delete_user_service_test.rb	`100% <100%> (ø)`
app/lib/event_store/repository.rb	`100% <100%> (ø)`	⬆️
app/services/segment/unexpected_response_error.rb	`100% <100%> (ø)`
... and 194 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f7fa1d...06ab167. Read the comment docs.

thomasmaas

Any way we can test this in preview/staging? (We do have an Intercom staging account, not sure about segment)

macejmic · 2019-03-22T15:03:24Z

app/lib/features/segment_deletion_config.rb

+    end
+
+    def self.config
+      configuration.config


this is an instance variable on class level? 🇫🇷

@macejmic Yes 😂 It was originally a module because we only need one global config for a feature, but @hallelujah explained me that it is better to have it like a class, the variable @configuration of the class (which is one of the instances) is the global configuration for the feature, and we can have as many instances as we want with different configs to test them in isolation without having to do dirty stuff from the tests like https://github.com/3scale/porta/pull/589/files#diff-02fd35c4c56ad40080442d610f0cc910L7

ok, that could work :)

Martouta · 2019-03-22T15:33:11Z

[OUTDATED COMMENT! 😉 ]
As I explained to @thomasmaas and in the sync to everyone before, we can't test this in preview because we would delete real data.
We can test it locally, carefully, with users that we KNOW for sure that should be deleted anyway 😄

…letedUserEvent is created When segment deletion feature is disabled, do not even enqueue the worker

1. Feature configs implementation 2. GBPRApiRequest implementation 3. Test of the worker

hallelujah · 2019-05-08T09:27:40Z

app/lib/features/segment_deletion_config.rb

+# frozen_string_literal: true
+
+module Features
+  class SegmentDeletionConfig < Config; end


Do you need this?

I think that I do because we have different types of features (right now AccountDeletionConfig and SegmentDeletionConfig) and we need to load them by type (look at the initializer) and fetch them where we use them by type... and even stub them by type. Almost the whole PR is based on that 🤔 check SegmentDeletionConfig and you will see 15 results so far 😄

sounds good to me.

macejmic · 2019-05-08T16:41:47Z

good job! 🍤

hallelujah

Looks good

Martouta self-assigned this Mar 21, 2019

thomasmaas reviewed Mar 21, 2019

View reviewed changes

test/events/users/user_deleted_event_test.rb Outdated

assert_equal event.metadata[:provider_id], master_account.id

end

test 'the event is persisted and with the necessary attributes when its associations are already destroyed' do

This comment was marked as resolved.

Sign in to view

thomasmaas reviewed Mar 21, 2019

View reviewed changes

test/unit/user_test.rb Outdated

@@ -20,6 +20,16 @@ class UserTest < ActiveSupport::TestCase

ActionMailer::Base.deliveries = []

end

test 'the event is created when the user is destroyed' do

This comment was marked as resolved.

Sign in to view

thomasmaas requested changes Mar 21, 2019

View reviewed changes

Martouta force-pushed the user-destroyed-segment-delete branch from 20ce617 to 2042605 Compare March 21, 2019 15:39

Martouta force-pushed the user-destroyed-segment-delete branch 7 times, most recently from 1fbeca9 to b442bb9 Compare March 22, 2019 09:47

Martouta changed the title ~~[WIP][NOT REVIEWABLE] Delete users in segment when they are deleted from system~~ [WIP] Delete users in segment when they are deleted from system Mar 22, 2019

Martouta requested a review from thomasmaas March 22, 2019 09:49

thomasmaas previously approved these changes Mar 22, 2019

View reviewed changes

Martouta dismissed thomasmaas’s stale review via 2c7c6f2 March 22, 2019 11:14

Martouta force-pushed the user-destroyed-segment-delete branch 3 times, most recently from 7b16e91 to a35cfc9 Compare March 22, 2019 14:05

Martouta mentioned this pull request Mar 22, 2019

[Refactor] AccountDeletionConfig belongs to Features namespace #721

Merged

Martouta force-pushed the user-destroyed-segment-delete branch 3 times, most recently from bc79dbb to d443efc Compare March 22, 2019 15:02

macejmic reviewed Mar 22, 2019

View reviewed changes

Martouta force-pushed the user-destroyed-segment-delete branch 2 times, most recently from 06276b6 to 378b905 Compare March 22, 2019 15:31

Martouta changed the title ~~[WIP] Delete users in segment when they are deleted from system~~ Delete users in segment when they are deleted from system Mar 22, 2019

Martouta force-pushed the user-destroyed-segment-delete branch from 497e89b to 974cede Compare April 25, 2019 08:40

macejmic previously approved these changes Apr 25, 2019

View reviewed changes

Martouta requested a review from guicassolato April 25, 2019 08:41

Martouta dismissed macejmic’s stale review via a30bd83 May 6, 2019 14:14

Martouta force-pushed the user-destroyed-segment-delete branch from 974cede to a30bd83 Compare May 6, 2019 14:14

Martouta requested review from macejmic and a team May 6, 2019 15:32

Martouta changed the title ~~[WIP] Delete users in segment when they are deleted from system~~ Delete users in segment when they are deleted from system May 6, 2019

Martouta changed the title ~~Delete users in segment when they are deleted from system~~ [WIP] Delete users in segment when they are deleted from system May 6, 2019

Martouta mentioned this pull request May 6, 2019

[WIP][DO NOT EVER MERGE THIS PR] Checking what is wrong in CircleCI for #717 #785

Closed

Martouta force-pushed the user-destroyed-segment-delete branch from 71a8322 to cf4dd10 Compare May 6, 2019 16:50

Martouta added 4 commits May 7, 2019 11:08

[Feature] SegmentDeletionConfig

3a117c7

When a User is destroyed, save a simple UserDeletedEvent

d13d7aa

Segment Service to request token

5c6e297

Segment Service to delete user

f106225

Martouta force-pushed the user-destroyed-segment-delete branch from cf4dd10 to 1bde827 Compare May 7, 2019 09:08

Martouta changed the title ~~[WIP] Delete users in segment when they are deleted from system~~ Delete users in segment when they are deleted from system May 7, 2019

Martouta removed the do not review This PR is currently not reviewable label May 7, 2019

Martouta added 3 commits May 7, 2019 18:17

Subscription & Worker to delete user in segment in background when De…

ba5cbf6

…letedUserEvent is created When segment deletion feature is disabled, do not even enqueue the worker

[Refactor]

41c8978

1. Feature configs implementation 2. GBPRApiRequest implementation 3. Test of the worker

Remove SegmentSubscriber for Account + Remove first_admin_id

06ab167

Martouta force-pushed the user-destroyed-segment-delete branch from 1bde827 to 06ab167 Compare May 7, 2019 16:17

hallelujah reviewed May 8, 2019

View reviewed changes

Martouta requested review from hallelujah and a team May 8, 2019 09:33

macejmic approved these changes May 8, 2019

View reviewed changes

hallelujah approved these changes May 9, 2019

View reviewed changes

Martouta merged commit 7ebf9c7 into master May 9, 2019

Martouta deleted the user-destroyed-segment-delete branch May 9, 2019 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete users in segment when they are deleted from system #717

Delete users in segment when they are deleted from system #717

Martouta commented Mar 21, 2019 •

edited

Loading

thomasmaas Mar 21, 2019

thomasmaas Mar 21, 2019

Martouta Mar 22, 2019 •

edited

Loading

macejmic Mar 22, 2019

This comment was marked as resolved.

This comment was marked as resolved.

thomasmaas left a comment

codecov bot commented Mar 21, 2019 •

edited

Loading

thomasmaas left a comment •

edited

Loading

macejmic Mar 22, 2019

Martouta Mar 22, 2019

macejmic Mar 22, 2019

Martouta commented Mar 22, 2019 •

edited

Loading

hallelujah May 8, 2019

Martouta May 8, 2019

macejmic May 8, 2019

macejmic commented May 8, 2019

hallelujah left a comment

	def destroyable?
	return true if destroyed_by_association
	!(account && !account.destroyed? && account.admins == [self])
	end

		@@ -0,0 +1,5 @@
		# frozen_string_literal: true

		class UserRelatedEvent < BaseEventStoreEvent

Delete users in segment when they are deleted from system #717

Delete users in segment when they are deleted from system #717

Conversation

Martouta commented Mar 21, 2019 • edited Loading

Tasks of this PR

Why to remove the SegmentSubscriber

Why to remove the first_admin_id

In preview only seen that the configuration is correct

Verification steps (locally and with real production data, so be careful with this!)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martouta Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

thomasmaas left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2019 • edited Loading

Codecov Report

thomasmaas left a comment • edited Loading

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

Martouta commented Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macejmic commented May 8, 2019

hallelujah left a comment

Choose a reason for hiding this comment

Martouta commented Mar 21, 2019 •

edited

Loading

Martouta Mar 22, 2019 •

edited

Loading

codecov bot commented Mar 21, 2019 •

edited

Loading

thomasmaas left a comment •

edited

Loading

Martouta commented Mar 22, 2019 •

edited

Loading