From 6f8244e3c6662814be569454f9254c84c58bafb2 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Wed, 27 Nov 2024 17:19:46 +0100 Subject: [PATCH 1/5] feat(projects): add method to find next relevant page #1191 Signed-off-by: David Wallace --- rdmo/projects/progress.py | 57 ++++++++++++++++++++++++++++++--------- rdmo/projects/viewsets.py | 45 +++++++++++++++++++------------ 2 files changed, 72 insertions(+), 30 deletions(-) diff --git a/rdmo/projects/progress.py b/rdmo/projects/progress.py index efe7421369..372873ab07 100644 --- a/rdmo/projects/progress.py +++ b/rdmo/projects/progress.py @@ -47,6 +47,48 @@ def compute_sets(values): return sets +def compute_next_relevant_page(current_page, direction, catalog, resolved_conditions): + # compute the next relevant page based on resolved conditions. + max_iterations = len(catalog.pages) + iterations = 0 + + while iterations < max_iterations: + iterations += 1 + + # Determine the next page based on the specified direction + next_page = (catalog.get_prev_page(current_page) if direction == 'prev' + else catalog.get_next_page(current_page)) + + # If no further pages are available, return None + if not next_page: + return None + + # Use compute_show_page with precomputed resolved_conditions to check if the next page meets conditions + if compute_show_page(next_page, resolved_conditions): + return next_page # Found the next relevant page + + # Move to the next page in sequence if conditions are not met + current_page = next_page + return None + +def compute_show_page(page, conditions): + """Determine if a page should be shown based on resolved conditions.""" + # show only pages with resolved conditions, but show all pages without conditions + pages_conditions = {page.id for page in page.conditions.all()} + + if pages_conditions: + # check if any valuesets for set_prefix = '' resolved + # for non collection pages restrict further to set_index = 0 + + return any( + (set_prefix == '') and (page.is_collection or set_index == 0) + for page_condition in pages_conditions + for set_prefix, set_index in conditions[page_condition] + ) + else: + return True + + def compute_navigation(section, project, snapshot=None): # get all values for this project and snapshot values = project.values.filter(snapshot=snapshot).select_related('attribute', 'option') @@ -74,19 +116,8 @@ def compute_navigation(section, project, snapshot=None): navigation_section['pages'] = [] for page in catalog_section.elements: - pages_conditions = {page.id for page in page.conditions.all()} - - # show only pages with resolved conditions, but show all pages without conditions - if pages_conditions: - # check if any valuesets for set_prefix = '' resolved - # for non collection pages restrict further to set_index = 0 - show = any( - (set_prefix == '') and (page.is_collection or set_index == 0) - for page_condition in pages_conditions - for set_prefix, set_index in conditions[page_condition] - ) - else: - show = True + + show = compute_show_page(page, conditions) # count the total number of questions, taking sets and conditions into account counts = count_questions(page, sets, conditions) diff --git a/rdmo/projects/viewsets.py b/rdmo/projects/viewsets.py index 8c5d367a5b..b01a3b9926 100644 --- a/rdmo/projects/viewsets.py +++ b/rdmo/projects/viewsets.py @@ -42,7 +42,14 @@ HasProjectProgressObjectPermission, HasProjectsPermission, ) -from .progress import compute_navigation, compute_progress +from .progress import ( + compute_navigation, + compute_next_relevant_page, + compute_progress, + compute_sets, + compute_show_page, + resolve_conditions, +) from .serializers.v1 import ( IntegrationSerializer, InviteSerializer, @@ -521,29 +528,33 @@ def dispatch(self, *args, **kwargs): def retrieve(self, request, *args, **kwargs): page = self.get_object() - conditions = page.conditions.select_related('source', 'target_option') - + catalog = self.project.catalog values = self.project.values.filter(snapshot=None).select_related('attribute', 'option') - if check_conditions(conditions, values): + sets = compute_sets(values) + resolved_conditions = resolve_conditions(catalog, values, sets) + + # check if the current page meets conditions + if compute_show_page(page, resolved_conditions): serializer = self.get_serializer(page) return Response(serializer.data) else: - if request.GET.get('back') == 'true': - prev_page = self.project.catalog.get_prev_page(page) - if prev_page is not None: - url = reverse('v1-projects:project-page-detail', - args=[self.project.id, prev_page.id]) + '?back=true' - return HttpResponseRedirect(url, status=303) - else: - next_page = self.project.catalog.get_next_page(page) - if next_page is not None: - url = reverse('v1-projects:project-page-detail', args=[self.project.id, next_page.id]) - return HttpResponseRedirect(url, status=303) - - # indicate end of catalog + # determine the direction of navigation (previous or next) + direction = 'prev' if request.GET.get('back') == 'true' else 'next' + + # find the next relevant page with from pages and resolved conditions + next_page = compute_next_relevant_page(page, direction, catalog, resolved_conditions) + + if next_page is not None: + url = reverse('v1-projects:project-page-detail', args=[self.project.id, next_page.id]) + if direction == 'prev': + url += '?back=true' + return HttpResponseRedirect(url, status=303) + + # end of catalog, if no next relevant page is found return Response(status=204) + @action(detail=False, url_path='continue', permission_classes=(HasModelPermission | HasProjectPagePermission, )) def get_continue(self, request, pk=None, parent_lookup_project=None): try: From db39d47879799e09a6f73701edf2115eb2f8d6a5 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Fri, 29 Nov 2024 17:04:22 +0100 Subject: [PATCH 2/5] refactor(projects,progress): compute next relevant page recursively Signed-off-by: David Wallace --- rdmo/projects/progress.py | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/rdmo/projects/progress.py b/rdmo/projects/progress.py index 372873ab07..01036caceb 100644 --- a/rdmo/projects/progress.py +++ b/rdmo/projects/progress.py @@ -49,27 +49,23 @@ def compute_sets(values): def compute_next_relevant_page(current_page, direction, catalog, resolved_conditions): # compute the next relevant page based on resolved conditions. - max_iterations = len(catalog.pages) - iterations = 0 + # If no current_page, return None + if not current_page: + return None - while iterations < max_iterations: - iterations += 1 + # Check if the current page meets the conditions + if compute_show_page(current_page, resolved_conditions): + return current_page - # Determine the next page based on the specified direction - next_page = (catalog.get_prev_page(current_page) if direction == 'prev' - else catalog.get_next_page(current_page)) + # Determine the next page based on the specified direction + next_page = ( + catalog.get_prev_page(current_page) if direction == 'prev' + else catalog.get_next_page(current_page) + ) - # If no further pages are available, return None - if not next_page: - return None + # Recursive step: Check the next page + return compute_next_relevant_page(next_page, direction, catalog, resolved_conditions) - # Use compute_show_page with precomputed resolved_conditions to check if the next page meets conditions - if compute_show_page(next_page, resolved_conditions): - return next_page # Found the next relevant page - - # Move to the next page in sequence if conditions are not met - current_page = next_page - return None def compute_show_page(page, conditions): """Determine if a page should be shown based on resolved conditions.""" From 5c492e366805e913b0dcd21ee3b0ba7b6df3889e Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 2 Dec 2024 18:56:21 +0100 Subject: [PATCH 3/5] tests(project, page): add specific test for next relevant page Signed-off-by: David Wallace --- .../tests/test_viewset_project_page.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/rdmo/projects/tests/test_viewset_project_page.py b/rdmo/projects/tests/test_viewset_project_page.py index c9d0510a50..8a6b326f4f 100644 --- a/rdmo/projects/tests/test_viewset_project_page.py +++ b/rdmo/projects/tests/test_viewset_project_page.py @@ -88,3 +88,37 @@ def test_detail_page_with_nested_questionsets(db, client): assert questionsets_ids == nested_questionsets_id element_ids = [i['id'] for qs in questionsets for i in qs['elements']] assert element_ids == nested_element_ids + + +@pytest.mark.parametrize('direction', ['next', 'prev']) +def test_detail_page_resolve_next_relevant_page(db, client, direction): + project_id = 1 + username = 'owner' + start_page_id = 64 + end_page_id = 69 + + client.login(username=username, password=username) + + if direction == 'next': + next_page_id = start_page_id + 1 + add_url = '' + else: # direction == 'prev': + start_page_id, end_page_id = end_page_id, start_page_id + next_page_id = start_page_id - 1 + add_url = '?back=true' + + # get the starting page + url = reverse(urlnames['detail'], args=[project_id, start_page_id]) + response = client.get(f'{url}{add_url}') + assert response.status_code == 200 + assert response.json().get(f'{direction}_page') == next_page_id + # get the following page, depending on direction + url_next = reverse(urlnames['detail'], args=[project_id, next_page_id]) + response_next = client.get(f'{url_next}{add_url}') + assert response_next.status_code == 303 + # this should show the redirect to the next relevant page + assert response_next.url.endswith(f'{end_page_id}/{add_url}') + + # a get on the redirected url as a double check + response_next_relevant = client.get(response_next.url) + assert response_next_relevant.status_code == 200 From e413d301fb6376ebe76d3af3c3466bcd70ab6efa Mon Sep 17 00:00:00 2001 From: David Wallace Date: Tue, 3 Dec 2024 17:30:57 +0100 Subject: [PATCH 4/5] refactor(projects,pages): update style Signed-off-by: David Wallace --- rdmo/projects/progress.py | 13 +++++++------ rdmo/projects/tests/test_viewset_project_page.py | 4 +++- rdmo/projects/viewsets.py | 12 +++++------- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/rdmo/projects/progress.py b/rdmo/projects/progress.py index 01036caceb..ce03d340b4 100644 --- a/rdmo/projects/progress.py +++ b/rdmo/projects/progress.py @@ -49,6 +49,13 @@ def compute_sets(values): def compute_next_relevant_page(current_page, direction, catalog, resolved_conditions): # compute the next relevant page based on resolved conditions. + + # Determine the next page based on the specified direction + next_page = ( + catalog.get_prev_page(current_page) if direction == 'prev' + else catalog.get_next_page(current_page) + ) + # If no current_page, return None if not current_page: return None @@ -57,12 +64,6 @@ def compute_next_relevant_page(current_page, direction, catalog, resolved_condit if compute_show_page(current_page, resolved_conditions): return current_page - # Determine the next page based on the specified direction - next_page = ( - catalog.get_prev_page(current_page) if direction == 'prev' - else catalog.get_next_page(current_page) - ) - # Recursive step: Check the next page return compute_next_relevant_page(next_page, direction, catalog, resolved_conditions) diff --git a/rdmo/projects/tests/test_viewset_project_page.py b/rdmo/projects/tests/test_viewset_project_page.py index 8a6b326f4f..228ee2e3d3 100644 --- a/rdmo/projects/tests/test_viewset_project_page.py +++ b/rdmo/projects/tests/test_viewset_project_page.py @@ -112,12 +112,14 @@ def test_detail_page_resolve_next_relevant_page(db, client, direction): response = client.get(f'{url}{add_url}') assert response.status_code == 200 assert response.json().get(f'{direction}_page') == next_page_id + # get the following page, depending on direction url_next = reverse(urlnames['detail'], args=[project_id, next_page_id]) response_next = client.get(f'{url_next}{add_url}') assert response_next.status_code == 303 + # this should show the redirect to the next relevant page - assert response_next.url.endswith(f'{end_page_id}/{add_url}') + assert response_next.url.endswith(f'{end_page_id}/') # a get on the redirected url as a double check response_next_relevant = client.get(response_next.url) diff --git a/rdmo/projects/viewsets.py b/rdmo/projects/viewsets.py index b01a3b9926..4d52e3bb6e 100644 --- a/rdmo/projects/viewsets.py +++ b/rdmo/projects/viewsets.py @@ -21,7 +21,7 @@ from rdmo.conditions.models import Condition from rdmo.core.permissions import HasModelPermission -from rdmo.core.utils import human2bytes, return_file_response +from rdmo.core.utils import human2bytes, is_truthy, return_file_response from rdmo.options.models import OptionSet from rdmo.questions.models import Catalog, Page, Question, QuestionSet from rdmo.tasks.models import Task @@ -540,15 +540,13 @@ def retrieve(self, request, *args, **kwargs): return Response(serializer.data) else: # determine the direction of navigation (previous or next) - direction = 'prev' if request.GET.get('back') == 'true' else 'next' + direction = 'prev' if is_truthy(request.GET.get('back')) else 'next' # find the next relevant page with from pages and resolved conditions - next_page = compute_next_relevant_page(page, direction, catalog, resolved_conditions) + next_relevant_page = compute_next_relevant_page(page, direction, catalog, resolved_conditions) - if next_page is not None: - url = reverse('v1-projects:project-page-detail', args=[self.project.id, next_page.id]) - if direction == 'prev': - url += '?back=true' + if next_relevant_page is not None: + url = reverse('v1-projects:project-page-detail', args=[self.project.id, next_relevant_page.id]) return HttpResponseRedirect(url, status=303) # end of catalog, if no next relevant page is found From 365b892eeffcaed18e8f3dd87f38d371c7a0d8ce Mon Sep 17 00:00:00 2001 From: David Wallace Date: Wed, 4 Dec 2024 09:45:31 +0100 Subject: [PATCH 5/5] refactor(projects,pages): rename variables, update comments Signed-off-by: David Wallace --- rdmo/projects/progress.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/rdmo/projects/progress.py b/rdmo/projects/progress.py index ce03d340b4..755203800d 100644 --- a/rdmo/projects/progress.py +++ b/rdmo/projects/progress.py @@ -48,28 +48,27 @@ def compute_sets(values): def compute_next_relevant_page(current_page, direction, catalog, resolved_conditions): - # compute the next relevant page based on resolved conditions. - - # Determine the next page based on the specified direction + # recursively compute the next relevant page based on resolved conditions. + # first, get the next page from the catalog based on the specified direction next_page = ( catalog.get_prev_page(current_page) if direction == 'prev' else catalog.get_next_page(current_page) ) - # If no current_page, return None - if not current_page: + # if there is no next page, return None + if not next_page: return None - # Check if the current page meets the conditions - if compute_show_page(current_page, resolved_conditions): - return current_page + # check if the next page meets the conditions + if compute_show_page(next_page, resolved_conditions): + return next_page - # Recursive step: Check the next page + # recursive step: check the next page return compute_next_relevant_page(next_page, direction, catalog, resolved_conditions) def compute_show_page(page, conditions): - """Determine if a page should be shown based on resolved conditions.""" + # determine if a page should be shown based on resolved conditions # show only pages with resolved conditions, but show all pages without conditions pages_conditions = {page.id for page in page.conditions.all()} @@ -114,6 +113,7 @@ def compute_navigation(section, project, snapshot=None): for page in catalog_section.elements: + # determine if a page should be shown or not show = compute_show_page(page, conditions) # count the total number of questions, taking sets and conditions into account