From a32c57c25a97c3f87c7eb7752a928ec43fd607ae Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 11:04:06 +0300 Subject: [PATCH 1/7] fix merged dataset permissions not applied on share --- onadata/libs/models/share_project.py | 7 ++++ .../libs/tests/models/test_share_project.py | 32 ++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/onadata/libs/models/share_project.py b/onadata/libs/models/share_project.py index d179d15972..88490069b3 100644 --- a/onadata/libs/models/share_project.py +++ b/onadata/libs/models/share_project.py @@ -30,6 +30,9 @@ def remove_xform_permissions(project, user, role): for xform in project.xform_set.all(): # pylint: disable=protected-access role._remove_obj_permissions(user, xform) + # Removed MergedXForm permissions if XForm is also a MergedXForm + if hasattr(xform, "mergedxform"): + role._remove_obj_permissions(user, xform.mergedxform) def remove_dataview_permissions(project, user, role): @@ -85,6 +88,10 @@ def save(self, **kwargs): role = ROLES.get(meta_perm[1]) role.add(self.user, xform) + # Set MergedXForm permissions if XForm is also a MergedXForm + if hasattr(xform, "mergedxform"): + role.add(self.user, xform.mergedxform) + for dataview in self.project.dataview_set.all(): if dataview.matches_parent: role.add(self.user, dataview.xform) diff --git a/onadata/libs/tests/models/test_share_project.py b/onadata/libs/tests/models/test_share_project.py index c869852618..f66e2e2460 100644 --- a/onadata/libs/tests/models/test_share_project.py +++ b/onadata/libs/tests/models/test_share_project.py @@ -1,9 +1,11 @@ """Tests for module onadata.libs.models.share_project""" from unittest.mock import patch, call +from pyxform.builder import create_survey_element_from_dict from onadata.apps.logger.models.data_view import DataView from onadata.apps.logger.models.project import Project +from onadata.apps.logger.models.merged_xform import MergedXForm from onadata.apps.logger.models.xform import XForm from onadata.apps.main.tests.test_base import TestBase from onadata.libs.models.share_project import ShareProject @@ -35,7 +37,7 @@ def setUp(self): project = Project.objects.create( name="Demo", organization=self.user, created_by=self.user ) - self._publish_markdown(md_xform, self.user, project) + self._publish_markdown(md_xform, self.user, project, id_string="a") self.dataview_form = XForm.objects.all().order_by("-pk")[0] DataView.objects.create( name="Demo", @@ -44,6 +46,27 @@ def setUp(self): matches_parent=True, columns=[], ) + # MergedXForm + self._publish_markdown(md_xform, self.user, project, id_string="b") + xf1 = XForm.objects.get(id_string="a") + xf2 = XForm.objects.get(id_string="b") + survey = create_survey_element_from_dict(xf1.json_dict()) + survey["id_string"] = "c" + survey["sms_keyword"] = survey["id_string"] + survey["title"] = "Merged XForm" + self.merged_xf = MergedXForm.objects.create( + id_string=survey["id_string"], + sms_id_string=survey["id_string"], + title=survey["title"], + user=self.user, + created_by=self.user, + is_merged_dataset=True, + project=self.project, + xml=survey.to_xml(), + json=survey.to_json(), + ) + self.merged_xf.xforms.add(xf1) + self.merged_xf.xforms.add(xf2) self.alice = self._create_user("alice", "Yuao8(-)") @patch("onadata.libs.models.share_project.safe_delete") @@ -58,6 +81,8 @@ def test_share(self, mock_safe_delete, mock_propagate): self.assertTrue(ManagerRole.user_has_role(self.alice, self.project)) self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform)) self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf.xform_ptr)) mock_propagate.assert_called_once_with(args=[self.project.pk]) # Cache is invalidated mock_safe_delete.assert_has_calls( @@ -74,16 +99,21 @@ def test_remove(self, mock_safe_delete, mock_propagate): ManagerRole.add(self.alice, self.project) ManagerRole.add(self.alice, self.xform) ManagerRole.add(self.alice, self.dataview_form) + ManagerRole.add(self.alice, self.merged_xf) + ManagerRole.add(self.alice, self.merged_xf.xform_ptr) self.assertTrue(ManagerRole.user_has_role(self.alice, self.project)) self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform)) self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.merged_xf.xform_ptr)) # Remove user instance = ShareProject(self.project, self.alice, "manager", True) instance.save() self.assertFalse(ManagerRole.user_has_role(self.alice, self.project)) self.assertFalse(ManagerRole.user_has_role(self.alice, self.xform)) self.assertFalse(ManagerRole.user_has_role(self.alice, self.dataview_form)) + self.assertFalse(ManagerRole.user_has_role(self.alice, self.merged_xf)) mock_propagate.assert_called_once_with(args=[self.project.pk]) # Cache is invalidated mock_safe_delete.assert_has_calls( From eede3fa43277c15bfb46c8221e51b3e73b6acace Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 11:11:22 +0300 Subject: [PATCH 2/7] add testcase --- onadata/libs/tests/models/test_share_project.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/onadata/libs/tests/models/test_share_project.py b/onadata/libs/tests/models/test_share_project.py index f66e2e2460..28d287d7c7 100644 --- a/onadata/libs/tests/models/test_share_project.py +++ b/onadata/libs/tests/models/test_share_project.py @@ -114,6 +114,9 @@ def test_remove(self, mock_safe_delete, mock_propagate): self.assertFalse(ManagerRole.user_has_role(self.alice, self.xform)) self.assertFalse(ManagerRole.user_has_role(self.alice, self.dataview_form)) self.assertFalse(ManagerRole.user_has_role(self.alice, self.merged_xf)) + self.assertFalse( + ManagerRole.user_has_role(self.alice, self.merged_xf.xform_ptr) + ) mock_propagate.assert_called_once_with(args=[self.project.pk]) # Cache is invalidated mock_safe_delete.assert_has_calls( From 3f2381ee6bf5565d301bd42ed2ca13ee28559299 Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 11:14:07 +0300 Subject: [PATCH 3/7] update comment --- onadata/libs/tests/models/test_share_project.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/onadata/libs/tests/models/test_share_project.py b/onadata/libs/tests/models/test_share_project.py index 28d287d7c7..f9b27c21f1 100644 --- a/onadata/libs/tests/models/test_share_project.py +++ b/onadata/libs/tests/models/test_share_project.py @@ -95,13 +95,13 @@ def test_share(self, mock_safe_delete, mock_propagate): @patch("onadata.libs.models.share_project.safe_delete") def test_remove(self, mock_safe_delete, mock_propagate): """A user is removed from a project""" - # Add user + # Simulate share project ManagerRole.add(self.alice, self.project) ManagerRole.add(self.alice, self.xform) ManagerRole.add(self.alice, self.dataview_form) ManagerRole.add(self.alice, self.merged_xf) ManagerRole.add(self.alice, self.merged_xf.xform_ptr) - + # Confirm project shared self.assertTrue(ManagerRole.user_has_role(self.alice, self.project)) self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform)) self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form)) From b1a60b666d08111358574354cbe71f03b4300413 Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 11:20:04 +0300 Subject: [PATCH 4/7] update docstring --- onadata/libs/tests/models/test_share_project.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/onadata/libs/tests/models/test_share_project.py b/onadata/libs/tests/models/test_share_project.py index f9b27c21f1..f60d299a24 100644 --- a/onadata/libs/tests/models/test_share_project.py +++ b/onadata/libs/tests/models/test_share_project.py @@ -73,7 +73,7 @@ def setUp(self): def test_share(self, mock_safe_delete, mock_propagate): """A project is shared with a user - Permissions assigned to project, xform and dataview + Permissions assigned to project, xform, mergedxform and dataview """ instance = ShareProject(self.project, self.alice, "manager") instance.save() @@ -94,7 +94,10 @@ def test_share(self, mock_safe_delete, mock_propagate): @patch("onadata.libs.models.share_project.safe_delete") def test_remove(self, mock_safe_delete, mock_propagate): - """A user is removed from a project""" + """A user is removed from a project + + Permissions removed from project, xform, mergedxform and dataview + """ # Simulate share project ManagerRole.add(self.alice, self.project) ManagerRole.add(self.alice, self.xform) From b19fddce0f9cf8fb7c41164b82e46b1859f0646a Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 15:39:26 +0300 Subject: [PATCH 5/7] add decorator for flaky test --- onadata/apps/api/tests/viewsets/test_xform_viewset.py | 1 + 1 file changed, 1 insertion(+) diff --git a/onadata/apps/api/tests/viewsets/test_xform_viewset.py b/onadata/apps/api/tests/viewsets/test_xform_viewset.py index a3996d96ed..c7310ed1f6 100644 --- a/onadata/apps/api/tests/viewsets/test_xform_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_xform_viewset.py @@ -710,6 +710,7 @@ def setUp(self): ) @patch("onadata.apps.api.viewsets.xform_viewset.send_message") + @flaky def test_replace_form_with_external_choices(self, mock_send_message): with HTTMock(enketo_mock): xls_file_path = os.path.join( From 668d084111dd6f804f9a0eb3ce3cf9a8ef87e5b4 Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 15:58:22 +0300 Subject: [PATCH 6/7] add flaky decorator to test --- onadata/apps/api/tests/viewsets/test_xform_viewset.py | 1 + 1 file changed, 1 insertion(+) diff --git a/onadata/apps/api/tests/viewsets/test_xform_viewset.py b/onadata/apps/api/tests/viewsets/test_xform_viewset.py index c7310ed1f6..a5969e631c 100644 --- a/onadata/apps/api/tests/viewsets/test_xform_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_xform_viewset.py @@ -2284,6 +2284,7 @@ def test_form_clone_shared_forms(self): self.assertEqual(response.status_code, 201) self.assertEqual(count + 1, XForm.objects.count()) + @flaky def test_return_error_on_clone_duplicate(self): with HTTMock(enketo_mock): self._publish_xls_form_to_project() From 18a8854c5595f14133afaa447fe101d6e4b30ae4 Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Mon, 20 May 2024 16:18:54 +0300 Subject: [PATCH 7/7] mark flaky test --- onadata/apps/api/tests/viewsets/test_data_viewset.py | 2 +- onadata/apps/logger/tests/test_briefcase_client.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/onadata/apps/api/tests/viewsets/test_data_viewset.py b/onadata/apps/api/tests/viewsets/test_data_viewset.py index 223925aae3..d30b708283 100644 --- a/onadata/apps/api/tests/viewsets/test_data_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_data_viewset.py @@ -3721,7 +3721,7 @@ def setUp(self): self.logger = logging.getLogger("console_logger") # pylint: disable=invalid-name,too-many-locals - @flaky(max_runs=5) + @flaky(max_runs=8) def test_data_retrieve_instance_osm_format(self): """Test /data endpoint OSM format.""" filenames = [ diff --git a/onadata/apps/logger/tests/test_briefcase_client.py b/onadata/apps/logger/tests/test_briefcase_client.py index 82d30fd855..ead683b0d9 100644 --- a/onadata/apps/logger/tests/test_briefcase_client.py +++ b/onadata/apps/logger/tests/test_briefcase_client.py @@ -16,6 +16,7 @@ import requests import requests_mock from django_digest.test import Client as DigestClient +from flaky import flaky from six.moves.urllib.parse import urljoin from onadata.apps.logger.models import Instance, XForm @@ -168,6 +169,7 @@ def _download_submissions(self): mocker.head(requests_mock.ANY, content=submission_list) self.briefcase_client.download_instances(self.xform.id_string) + @flaky(max_runs=8) def test_download_xform_xml(self): """ Download xform via briefcase api