Milimetric has submitted this change and it was merged.
Change subject: Add ability to remove cohorts from database.
......................................................................
Add ability to remove cohorts from database.
Owners can remove their cohorts from the database. A warning will show if there
are other users sharing the cohort. When deleted, viewers records will be
deleted from their personal list. If a viewer deletes a cohort, it will only
delete the cohort from their personal list, not from the cohort database.
Change-Id: I431cad933d5b888f10fe5533b75e7b1776649ab3
---
M tests/test_controllers/test_cohorts.py
M wikimetrics/controllers/cohorts.py
M wikimetrics/exceptions.py
M wikimetrics/static/js/cohortList.js
M wikimetrics/static/js/site.js
M wikimetrics/templates/cohorts.html
6 files changed, 246 insertions(+), 26 deletions(-)
Approvals:
Milimetric: Looks good to me, approved
jenkins-bot: Verified
diff --git a/tests/test_controllers/test_cohorts.py
b/tests/test_controllers/test_cohorts.py
index e86df9d..e8cb362 100644
--- a/tests/test_controllers/test_cohorts.py
+++ b/tests/test_controllers/test_cohorts.py
@@ -1,12 +1,12 @@
import json
import time
from StringIO import StringIO
-from nose.tools import assert_equal, assert_true, assert_false, raises, nottest
+from nose.tools import assert_equal, assert_not_equal, assert_true,
assert_false
from wikimetrics.configurables import app
from tests.fixtures import WebTest
from wikimetrics.models import (
Cohort, CohortUser, CohortUserRole, ValidateCohort,
- CohortWikiUser, WikiUser,
+ CohortWikiUser, WikiUser, User
)
@@ -135,26 +135,127 @@
assert_true(
response.data.find('Validating cohort') >= 0
)
-
- def test_delete_cohort(self):
+
+ def test_delete_cohort_owner_no_viewer(self):
response = self.app.post('/cohorts/delete/{0}'.format(self.cohort.id))
-
+
assert_equal(response.status_code, 200)
assert_true(response.data.find('isRedirect') >= 0)
assert_true(response.data.find('/cohorts/') >= 0)
+ cohort_id = self.cohort.id
self.session.commit()
+
+ # Check that all relevant rows are deleted
+ cwu = self.session.query(CohortWikiUser) \
+ .filter(CohortWikiUser.cohort_id == cohort_id) \
+ .first()
+ assert_equal(cwu, None)
cu = self.session.query(CohortUser) \
- .filter(CohortUser.cohort_id == self.cohort.id) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .filter(CohortUser.user_id == self.owner_user_id) \
.first()
assert_equal(cu, None)
-
+ wu = self.session.query(WikiUser) \
+ .filter(WikiUser.validating_cohort == cohort_id) \
+ .first()
+ assert_equal(wu, None)
+ c = self.session.query(Cohort).get(cohort_id)
+ assert_equal(c, None)
+
+ def test_delete_cohort_owner_has_viewer(self):
+ viewer_user = User()
+ self.session.add(viewer_user)
+ self.session.commit()
+
+ viewer_cohort_user = CohortUser(
+ user_id=viewer_user.id,
+ cohort_id=self.cohort.id,
+ role=CohortUserRole.VIEWER
+ )
+ self.session.add(viewer_cohort_user)
+ self.session.commit()
+ response = self.app.post('/cohorts/delete/{0}'.format(self.cohort.id))
+
+ assert_equal(response.status_code, 200)
+ assert_true(response.data.find('isRedirect') >= 0)
+ assert_true(response.data.find('/cohorts/') >= 0)
+ cohort_id = self.cohort.id
+ self.session.commit()
+
+ # Check that all relevant rows are deleted
+ cwu = self.session.query(CohortWikiUser) \
+ .filter(CohortWikiUser.cohort_id == cohort_id) \
+ .first()
+ assert_equal(cwu, None)
+ cu = self.session.query(CohortUser) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .first()
+ assert_equal(cu, None)
+ wu = self.session.query(WikiUser) \
+ .filter(WikiUser.validating_cohort == cohort_id) \
+ .first()
+ assert_equal(wu, None)
+ c = self.session.query(Cohort).get(cohort_id)
+ assert_equal(c, None)
+
+ def test_delete_cohort_as_viewer(self):
+ # Changing the owner_user_id to a VIEWER
+ self.session.query(CohortUser) \
+ .filter(CohortUser.user_id == self.owner_user_id) \
+ .filter(CohortUser.cohort_id == self.cohort.id) \
+ .update({'role': CohortUserRole.VIEWER})
+ self.session.commit()
+
+ # Adding a different CohortUser as owner
+ new_cohort_user = CohortUser(
+ cohort_id=self.cohort.id,
+ role=CohortUserRole.OWNER
+ )
+ self.session.add(new_cohort_user)
+ self.session.commit()
+
+ response = self.app.post('/cohorts/delete/{0}'.format(self.cohort.id))
+
+ assert_equal(response.status_code, 200)
+ assert_true(response.data.find('isRedirect') >= 0)
+ assert_true(response.data.find('/cohorts/') >= 0)
+ cohort_id = self.cohort.id
+ self.session.commit()
+
+ # Check that all relevant rows are deleted
+ cwu = self.session.query(CohortWikiUser) \
+ .filter(CohortWikiUser.cohort_id == cohort_id) \
+ .first()
+ assert_not_equal(cwu, None)
+ cu = self.session.query(CohortUser) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .filter(CohortWikiUser.id == self.owner_user_id) \
+ .first()
+ assert_equal(cu, None)
+ wu = self.session.query(WikiUser) \
+ .filter(WikiUser.validating_cohort == cohort_id) \
+ .first()
+ assert_not_equal(wu, None)
+ c = self.session.query(Cohort).get(cohort_id)
+ assert_not_equal(c, None)
+
def test_delete_unauthorized_cohort(self):
self.session.query(CohortUser).delete()
self.session.commit()
response = self.app.post('/cohorts/delete/{0}'.format(self.cohort.id))
-
+
assert_true(response.data.find('isError') >= 0)
- assert_true(response.data.find('This Cohort can not be deleted') >= 0)
+ assert_true(response.data.find('No role found in cohort user.') >= 0)
+
+ def test_delete_cohort_database_error(self):
+ self.session.query(CohortWikiUser).delete()
+ self.session.commit()
+ response = self.app.post('/cohorts/delete/{0}'.format(self.cohort.id))
+
+ expected_message = 'Owner attempt to delete a cohort failed. ' \
+ 'Cannot delete CohortWikiUser.'
+ assert_true(response.data.find('isError') >= 0)
+ assert_true(response.data.find(expected_message) >= 0)
class CohortsControllerUploadTest(WebTest):
diff --git a/wikimetrics/controllers/cohorts.py
b/wikimetrics/controllers/cohorts.py
index f3eec14..984bff4 100644
--- a/wikimetrics/controllers/cohorts.py
+++ b/wikimetrics/controllers/cohorts.py
@@ -17,6 +17,7 @@
User, WikiUser, CohortWikiUser, MediawikiUser,
ValidateCohort
)
+from ..exceptions import DatabaseError
@app.route('/cohorts/')
@@ -127,6 +128,7 @@
cohort_dict['total_count'] = len(cohort_dict['wikiusers'])
cohort_dict['valid_count'] = cohort_dict['total_count']
cohort_dict['invalid_count'] = 0
+ cohort_dict['delete_message'] = None
return cohort_dict
validation_task = ValidateCohort.task.AsyncResult(task_key)
@@ -147,6 +149,14 @@
cohort_dict['total_count'] = session.query(func.count(WikiUser)) \
.filter(WikiUser.validating_cohort == cohort_dict['id']) \
.one()[0]
+ users = num_users(session, cohort_dict['id'])
+ non_owners = users - 1
+ role = get_role(session, cohort_dict['id'])
+ if users != 1 and role == CohortUserRole.OWNER:
+ cohort_dict['delete_message'] = 'delete this cohort? ' + \
+ 'There are {0} other user(s) shared on this
cohort.'.format(non_owners)
+ else:
+ cohort_dict['delete_message'] = 'delete this cohort?'
finally:
session.close()
return cohort_dict
@@ -227,23 +237,122 @@
session.close()
+def num_users(session, cohort_id):
+ user_count = session.query(CohortUser) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .count()
+ return user_count
+
+
+def get_role(session, cohort_id):
+ """
+ Returns the role of the current user.
+ """
+ try:
+ cohort_user = session.query(CohortUser.role) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .filter(CohortUser.user_id == current_user.id) \
+ .one()[0]
+ return cohort_user
+ except:
+ session.rollback()
+ raise DatabaseError('No role found in cohort user.')
+
+
+def delete_viewer_cohort(session, cohort_id):
+ """
+ Used when deleting a user's connection to a cohort. Currently used when
user
+ is a VIEWER of a cohort and want to remove that cohort from their list.
+
+ Raises exception when viewer is duplicated, nonexistent, or can not be
deleted.
+ """
+ cu = session.query(CohortUser) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .filter(CohortUser.user_id == current_user.id) \
+ .filter(CohortUser.role == CohortUserRole.VIEWER) \
+ .delete()
+
+ if cu != 1:
+ session.rollback()
+ raise DatabaseError('Viewer attempt delete cohort failed.')
+
+
+def delete_owner_cohort(session, cohort_id):
+ """
+ Deletes the cohort and all associate records with that cohort if user is
the
+ owner.
+
+ Raises an error if it cannot delete the cohort.
+ """
+ # Check that there's only one owner and delete it
+ cu = session.query(CohortUser) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .filter(CohortUser.role == CohortUserRole.OWNER) \
+ .delete()
+
+ if cu != 1:
+ session.rollback()
+ raise DatabaseError('No owner or multiple owners in cohort.')
+ else:
+ try:
+ # Delete all other non-owners from cohort_user
+ session.query(CohortUser) \
+ .filter(CohortUser.cohort_id == cohort_id) \
+ .delete()
+ cwu = session.query(CohortWikiUser) \
+ .filter(CohortWikiUser.cohort_id == cohort_id) \
+ .delete()
+ if cwu < 1:
+ raise DatabaseError('Cannot delete CohortWikiUser.')
+
+ wu = session.query(WikiUser) \
+ .filter(WikiUser.validating_cohort == cohort_id) \
+ .delete()
+ if wu < 1:
+ raise DatabaseError('Cannot delete WikiUser.')
+
+ c = session.query(Cohort) \
+ .filter(Cohort.id == cohort_id) \
+ .delete()
+ if c < 1:
+ raise DatabaseError('Cannot delete Cohort.')
+ except DatabaseError as e:
+ print "here3"
+ session.rollback()
+ raise DatabaseError('Owner attempt to delete a cohort failed. ' +
e.message)
+
+
@app.route('/cohorts/delete/<int:cohort_id>', methods=['POST'])
def delete_cohort(cohort_id):
"""
- Deletes a cohort and all its wikiusers if it belongs to only current_user
+ Deletes a cohort and all its associated links if it belongs to only
current_user
Removes the relationship between current_user and this cohort if it belongs
- to more than just current_user
+ to a user other than current_user
"""
session = db.get_session()
try:
- result = session.query(CohortUser) \
- .filter(CohortUser.cohort_id == cohort_id) \
- .filter(CohortUser.user_id == current_user.id) \
- .delete()
- session.commit()
- if result > 0:
+ owner_and_viewers = num_users(session, cohort_id)
+ role = get_role(session, cohort_id)
+
+ # Owner wants to delete, no other viewers or
+ # Owner wants to delete, have other viewers, delete from other
viewer's lists too
+ if owner_and_viewers >= 1 and role == CohortUserRole.OWNER:
+ delete_owner_cohort(session, cohort_id)
+ session.commit()
return json_redirect(url_for('cohorts_index'))
+
+ # Viewer wants to delete cohort from their list, doesn't delete cohort
from db;l,
+ elif owner_and_viewers > 1 and role == CohortUserRole.VIEWER:
+ delete_viewer_cohort(session, cohort_id)
+ session.commit()
+ return json_redirect(url_for('cohorts_index'))
+
+ # None of the other cases fit.
else:
- return json_error('This Cohort can not be deleted')
+ session.rollback()
+ return json_error('This Cohort can not be deleted.')
+ except DatabaseError as e:
+ session.rollback()
+ return json_error(e.message)
finally:
session.close()
diff --git a/wikimetrics/exceptions.py b/wikimetrics/exceptions.py
index a3dd27b..5b3710a 100644
--- a/wikimetrics/exceptions.py
+++ b/wikimetrics/exceptions.py
@@ -18,3 +18,10 @@
"""
pass
+
+
+class DatabaseError(Exception):
+ """
+ Thrown when database calls don't return an expected result.
+ """
+ pass
diff --git a/wikimetrics/static/js/cohortList.js
b/wikimetrics/static/js/cohortList.js
index e2d2968..9559a94 100644
--- a/wikimetrics/static/js/cohortList.js
+++ b/wikimetrics/static/js/cohortList.js
@@ -12,6 +12,7 @@
cohort.total_count(data.total_count);
cohort.validation_status(data.validation_status);
cohort.wikiusers(data.wikiusers);
+ cohort.delete_message(data.delete_message);
},
view: function(cohort, event, callback){
@@ -35,7 +36,7 @@
},
deleteCohort: function(cohort, event){
- if (site.confirmDanger(event)){
+ if (site.confirmDanger(event, true)){
$.post('/cohorts/delete/' + cohort.id)
.done(site.handleWith(function(){
site.showWarning('Something is wrong, you should be
redirected');
@@ -43,7 +44,7 @@
.fail(site.failure);
}
},
-
+
validateWikiusers: function(cohort, event){
if (site.confirmDanger(event)){
$.post('/cohorts/validate/' + cohort.id)
@@ -90,6 +91,7 @@
item.valid_count = ko.observable(0);
item.total_count = ko.observable(0);
item.validation_status = ko.observable();
+ item.delete_message = ko.observable();
});
}
});
diff --git a/wikimetrics/static/js/site.js b/wikimetrics/static/js/site.js
index a75218f..e13c06f 100644
--- a/wikimetrics/static/js/site.js
+++ b/wikimetrics/static/js/site.js
@@ -21,9 +21,9 @@
return true;
},
- confirmDanger: function(event){
+ confirmDanger: function(event, noQuestion){
var title = $(event.target).attr('title');
- return confirm('Are you sure you want to ' + title + '?');
+ return confirm('Are you sure you want to ' + title + (noQuestion ? ''
: '?') );
},
showError: function (message, permanent){
diff --git a/wikimetrics/templates/cohorts.html
b/wikimetrics/templates/cohorts.html
index acf4834..a8289ca 100644
--- a/wikimetrics/templates/cohorts.html
+++ b/wikimetrics/templates/cohorts.html
@@ -21,9 +21,13 @@
<div class="hero-unit">
<h5 data-bind="text: name"></h5>
<p data-bind="text: description"></p>
- <div data-bind="if: validated() && valid_count() > 0">
+ <div class="btn-group pull-left" data-bind="if: validated() &&
valid_count() > 0">
<a
data-bind="attr:{href:'{{url_for('reports_request')}}#'+id}"
class="btn btn-primary">Create Report</a>
+ </div>
+ <div class="btn-group">
+ <a data-bind="click: $root.deleteCohort, attr: { title:
delete_message }"
+ class="btn btn-danger">Remove Cohort</a>
</div>
<div data-bind="if: validated_count() === total_count() &&
!validated()">
<hr/>
@@ -44,9 +48,6 @@
<div data-bind="if: valid_count() < total_count()">
<hr/>
<p>
- <a data-bind="click: $root.deleteCohort"
- class="btn btn-danger"
- title="remove this cohort from your list">Remove
Cohort</a>
<a data-bind="click: $root.validateWikiusers"
class="btn btn-primary"
title="cancel and restart validation for this
cohort">Validate Again</a>
--
To view, visit https://gerrit.wikimedia.org/r/119343
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I431cad933d5b888f10fe5533b75e7b1776649ab3
Gerrit-PatchSet: 19
Gerrit-Project: analytics/wikimetrics
Gerrit-Branch: master
Gerrit-Owner: Terrrydactyl <tcho...@gmail.com>
Gerrit-Reviewer: Milimetric <dandree...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
