From 69698a4068a2790f616f03918b816751f92e76dd Mon Sep 17 00:00:00 2001 From: Tasawer Nawaz Date: Wed, 24 Apr 2019 17:36:29 +0500 Subject: [PATCH] mark channel and percolatequery is deleted and update memeberships (#4289) --- discussions/api.py | 6 +-- ..._channel_deleted_and_update_memberships.py | 44 +++++++++++++++++ ...nel_deleted_and_update_memberships_test.py | 48 +++++++++++++++++++ .../migrations/0006_channel_is_deleted.py | 18 +++++++ discussions/models.py | 1 + discussions/tasks.py | 25 ++++++++++ discussions/tasks_test.py | 10 ++++ discussions/utils.py | 10 ++++ discussions/utils_test.py | 13 ++++- search/api.py | 4 +- search/indexing_api.py | 4 +- .../0005_percolatequery_is_deleted.py | 18 +++++++ search/models.py | 1 + search/tasks.py | 2 +- 14 files changed, 194 insertions(+), 10 deletions(-) create mode 100644 discussions/management/commands/mark_channel_deleted_and_update_memberships.py create mode 100644 discussions/management/commands/mark_channel_deleted_and_update_memberships_test.py create mode 100644 discussions/migrations/0006_channel_is_deleted.py create mode 100644 search/migrations/0005_percolatequery_is_deleted.py diff --git a/discussions/api.py b/discussions/api.py index ed3f8c93ef..50ec64e286 100644 --- a/discussions/api.py +++ b/discussions/api.py @@ -23,6 +23,7 @@ ModeratorSyncException, SubscriberSyncException, ) +from discussions.utils import get_moderators_for_channel from roles.models import Role from roles.roles import Permissions from search.api import adjust_search_for_percolator @@ -448,10 +449,7 @@ def add_moderators_to_channel(channel_name): Args: channel_name (str): The name of the channel """ - mod_ids = Role.objects.filter( - role__in=Role.permission_to_roles[Permissions.CAN_CREATE_FORUMS], - program__channelprogram__channel__name=channel_name, - ).values_list('user', flat=True) + mod_ids = get_moderators_for_channel(channel_name) for mod_id in mod_ids: discussion_user = create_or_update_discussion_user(mod_id) diff --git a/discussions/management/commands/mark_channel_deleted_and_update_memberships.py b/discussions/management/commands/mark_channel_deleted_and_update_memberships.py new file mode 100644 index 0000000000..2a206796db --- /dev/null +++ b/discussions/management/commands/mark_channel_deleted_and_update_memberships.py @@ -0,0 +1,44 @@ +""" +Mark Channel and PercolateQuery deleted and updates all the memberships to is_member=False, need_update=True +""" +from django.core.management import BaseCommand, CommandError + +from discussions.models import Channel +from discussions.tasks import remove_moderators_from_channel + + +class Command(BaseCommand): + """ + Mark Channel and PercolateQuery deleted and updates all the memberships to is_member=False, need_update=True + """ + help = 'Mark Channel and PercolateQuery deleted and updates all the memberships to ' \ + 'is_member=False, need_update=True.' + + def add_arguments(self, parser): + parser.add_argument('channel_name', type=str) + + def handle(self, *args, **kwargs): # pylint: disable=unused-argument + channel_name = kwargs.get('channel_name') + + try: + channel = Channel.objects.get(name=channel_name) + except Channel.DoesNotExist: + raise CommandError('Channel does not exists with name={channel_name}'.format(channel_name=channel_name)) + + channel.is_deleted = True + channel.save() + + percolate_query = channel.query + percolate_query.is_deleted = True + percolate_query.save() + + percolate_query.percolate_memberships.update(is_member=False, needs_update=True) + + self.stdout.write( + self.style.SUCCESS('Channel and PercolateQuery marked as deleted and related memberships are update.') + ) + + remove_moderators_from_channel.delay(channel_name) + self.stdout.write( + self.style.SUCCESS('Async job to remove moderators is submitted') + ) diff --git a/discussions/management/commands/mark_channel_deleted_and_update_memberships_test.py b/discussions/management/commands/mark_channel_deleted_and_update_memberships_test.py new file mode 100644 index 0000000000..f9d0582fef --- /dev/null +++ b/discussions/management/commands/mark_channel_deleted_and_update_memberships_test.py @@ -0,0 +1,48 @@ +"""Tests for management command `mark_channel_deleted_and_update_memberships`""" +import pytest +from django.core.management import call_command, CommandError + +from discussions.factories import ChannelFactory +from search.factories import PercolateQueryMembershipFactory + +pytestmark = [ + pytest.mark.django_db, +] + + +def test_without_argument(): + """Tests that commands raises commands error if required argument not provided. """ + with pytest.raises(CommandError) as ex: + call_command('mark_channel_deleted_and_update_memberships') + + assert str(ex.value) == 'Error: the following arguments are required: channel_name' + + +def test_channel_name_does_not_exist(): + """Test that command raises an error if channel name does not exists.""" + fake_name = 'fake_name' + with pytest.raises(CommandError) as ex: + call_command('mark_channel_deleted_and_update_memberships', 'fake_name') + + assert str(ex.value) == 'Channel does not exists with name={}'.format(fake_name) + + +def test_channel_marked_deleted(): + """ Test that command will mark channel as deleted.""" + + channel = ChannelFactory() + membership = PercolateQueryMembershipFactory.create(query=channel.query, is_member=True, needs_update=False) + + assert not channel.is_deleted + assert not channel.query.is_deleted + + call_command('mark_channel_deleted_and_update_memberships', channel.name) + + channel.refresh_from_db() + assert channel.is_deleted + assert channel.query.is_deleted + + # is_member and needs_update should be updated + membership.refresh_from_db() + assert not membership.is_member + assert membership.needs_update diff --git a/discussions/migrations/0006_channel_is_deleted.py b/discussions/migrations/0006_channel_is_deleted.py new file mode 100644 index 0000000000..2a920b7ccb --- /dev/null +++ b/discussions/migrations/0006_channel_is_deleted.py @@ -0,0 +1,18 @@ +# Generated by Django 2.1.5 on 2019-03-22 07:29 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('discussions', '0005_timestamped_discussions_models'), + ] + + operations = [ + migrations.AddField( + model_name='channel', + name='is_deleted', + field=models.BooleanField(default=False), + ), + ] diff --git a/discussions/models.py b/discussions/models.py index 11b7402331..a07749f8c7 100644 --- a/discussions/models.py +++ b/discussions/models.py @@ -27,6 +27,7 @@ class Channel(TimestampedModel): """ name = models.TextField(unique=True) query = models.ForeignKey(PercolateQuery, null=True, on_delete=models.SET_NULL, related_name='channels') + is_deleted = models.BooleanField(default=False) def __str__(self): return "Channel: {}".format(self.name) diff --git a/discussions/tasks.py b/discussions/tasks.py index d323d2d311..ab93b16acd 100644 --- a/discussions/tasks.py +++ b/discussions/tasks.py @@ -8,11 +8,13 @@ from discussions import api from discussions.models import ChannelProgram, DiscussionUser from discussions.exceptions import DiscussionUserSyncException +from discussions.utils import get_moderators_for_channel from micromasters.celery import app from micromasters.locks import Lock from micromasters.utils import now_in_utc from profiles.models import Profile + SYNC_MEMBERSHIPS_LOCK_NAME = 'discussions.tasks.sync_memberships_lock' # let it run for up to 2 minutes minus a 5 second buffer @@ -129,6 +131,29 @@ def add_user_as_moderator_to_channel(user_id, program_id): ) +@app.task() +def remove_moderators_from_channel(channel_name): + """ + Remove moderators of a given channel_name. + Args: + channel_name(str): channel name + """ + + mod_ids = get_moderators_for_channel(channel_name) + + for mod_id in mod_ids: + try: + discussion_user = DiscussionUser.objects.get(user_id=mod_id) + except DiscussionUser.DoesNotExist: + log.info('User: %d does not exist in discussion.', mod_id) + continue + + api.remove_moderator_from_channel( + channel_name, + discussion_user.username + ) + + @app.task() def remove_user_as_moderator_from_channel(user_id, program_id): """ diff --git a/discussions/tasks_test.py b/discussions/tasks_test.py index 39146214cf..2f9a3750b5 100644 --- a/discussions/tasks_test.py +++ b/discussions/tasks_test.py @@ -318,3 +318,13 @@ def test_remove_moderator_to_channel_no_feature_flag(settings, mocker): discussion_user = DiscussionUserFactory.create() tasks.remove_user_as_moderator_from_channel.delay(discussion_user.user_id, program.id) assert stub.called is False + + +def test_remove_moderators_from_channel(mocker): + """remove_moderators_from_channel should forward all arguments to the api function""" + stub = mocker.patch('discussions.api.remove_moderator_from_channel', autospec=True) + program = ChannelProgramFactory.create().program + with mute_signals(post_save): + discussion_user = DiscussionUserFactory.create() + tasks.remove_user_as_moderator_from_channel.delay(discussion_user.user_id, program.id) + assert stub.called is True diff --git a/discussions/utils.py b/discussions/utils.py index 91fa0168d9..9de6f777ab 100644 --- a/discussions/utils.py +++ b/discussions/utils.py @@ -4,6 +4,8 @@ from discussions import api from discussions.models import DiscussionUser +from roles.models import Role +from roles.roles import Permissions def get_token_for_user(user, force_create=False): @@ -60,3 +62,11 @@ def get_token_for_request(request, force_create=False): str: the token or None """ return get_token_for_user(request.user, force_create=force_create) + + +def get_moderators_for_channel(channel_name): + """ Return moderator ids against a given channel name.""" + return Role.objects.filter( + role__in=Role.permission_to_roles[Permissions.CAN_CREATE_FORUMS], + program__channelprogram__channel__name=channel_name, + ).values_list('user', flat=True) diff --git a/discussions/utils_test.py b/discussions/utils_test.py index fadcacc740..ae72252d44 100644 --- a/discussions/utils_test.py +++ b/discussions/utils_test.py @@ -7,9 +7,11 @@ import pytest from factory.django import mute_signals +from discussions.factories import ChannelProgramFactory from discussions.models import DiscussionUser -from discussions.utils import get_token_for_user +from discussions.utils import get_token_for_user, get_moderators_for_channel from micromasters.factories import UserFactory +from roles.factories import RoleFactory pytestmark = pytest.mark.django_db @@ -97,3 +99,12 @@ def test_get_token_for_user_force_discussion_user(settings, mocker): } ) assert mock_create_user.called_once_with(user.id) + + +def test_get_moderators_for_channel(): + """Test that method return list of moderator ids against given channel name.""" + channel_program = ChannelProgramFactory.create() + expected_moderators = {RoleFactory.create(program=channel_program.program).user.id for _ in range(5)} + + moderators = get_moderators_for_channel(channel_program.channel.name) + assert expected_moderators == set(moderators) diff --git a/search/api.py b/search/api.py index eac9f3d622..bf8b978109 100644 --- a/search/api.py +++ b/search/api.py @@ -262,7 +262,7 @@ def search_percolate_queries(program_enrollment_id, source_type): """ enrollment = ProgramEnrollment.objects.get(id=program_enrollment_id) result_ids = _search_percolate_queries(enrollment) - return PercolateQuery.objects.filter(id__in=result_ids, source_type=source_type) + return PercolateQuery.objects.filter(id__in=result_ids, source_type=source_type).exclude(is_deleted=True) def _search_percolate_queries(program_enrollment): @@ -360,7 +360,7 @@ def update_percolate_memberships(user, source_type): source_type (str): The type of the percolate query to filter on """ # ensure we have a membership for each of the queries so we can acquire a lock on them - percolate_queries = list(PercolateQuery.objects.filter(source_type=source_type)) + percolate_queries = list(PercolateQuery.objects.filter(source_type=source_type).exclude(is_deleted=True)) membership_ids = _ensure_memberships_for_queries( percolate_queries, user diff --git a/search/indexing_api.py b/search/indexing_api.py index 9b287904c7..f60767f45c 100644 --- a/search/indexing_api.py +++ b/search/indexing_api.py @@ -603,9 +603,9 @@ def recreate_index(): private_indices=[new_backing_private_index], ) - log.info("Indexing %d percolator queries...", PercolateQuery.objects.count()) + log.info("Indexing %d percolator queries...", PercolateQuery.objects.exclude(is_deleted=True).count()) _index_chunks( - _get_percolate_documents(PercolateQuery.objects.iterator()), + _get_percolate_documents(PercolateQuery.objects.exclude(is_deleted=True).iterator()), index=new_backing_percolate_index, ) diff --git a/search/migrations/0005_percolatequery_is_deleted.py b/search/migrations/0005_percolatequery_is_deleted.py new file mode 100644 index 0000000000..ff619b3984 --- /dev/null +++ b/search/migrations/0005_percolatequery_is_deleted.py @@ -0,0 +1,18 @@ +# Generated by Django 2.1.5 on 2019-03-22 07:29 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('search', '0004_add_percolate_query_membership'), + ] + + operations = [ + migrations.AddField( + model_name='percolatequery', + name='is_deleted', + field=models.BooleanField(default=False), + ), + ] diff --git a/search/models.py b/search/models.py index 430637d565..ff27eb52ed 100644 --- a/search/models.py +++ b/search/models.py @@ -19,6 +19,7 @@ class PercolateQuery(TimestampedModel): original_query = JSONField() query = JSONField() source_type = models.CharField(max_length=255, choices=[(choice, choice) for choice in SOURCE_TYPES]) + is_deleted = models.BooleanField(default=False) def __str__(self): return "Percolate query {}: {}".format(self.id, self.query) diff --git a/search/tasks.py b/search/tasks.py index 3427dc7605..6b99ae0859 100644 --- a/search/tasks.py +++ b/search/tasks.py @@ -113,7 +113,7 @@ def index_percolate_queries(percolate_query_ids): percolate_query_ids (iterable of int): Database ids for PercolateQuery instances to index """ - _index_percolate_queries(PercolateQuery.objects.filter(id__in=percolate_query_ids)) + _index_percolate_queries(PercolateQuery.objects.filter(id__in=percolate_query_ids).exclude(is_deleted=True)) @app.task