[OSDEV-1346] Add moderation-event GET endpoint

[VadimKovalenkoSNF]

Fix OSDEV-1346

Add GET api/v1/moderation-events

[barecheck]

React App | Jest test suite - Code coverage report

Total: 26.93%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[barecheck]

Dedupe Hub App | Unittest test suite - Code coverage report

Total: 56.14%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[barecheck]

Contricleaner App | Unittest test suite - Code coverage report

Total: 98.91%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[barecheck]

Countries App | Unittest test suite - Code coverage report

Total: 100%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

[barecheck]

Django App | Unittest test suite - Code coverage report

Total: 77.53%

Your code coverage diff: -0.38% ▾

✅ All code changes are covered

[coderabbitai]

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)

src/tests/v1/test_moderation_events.py (2)

15-63: Add edge cases to filter tests

The filter tests should also verify:

1. Empty result sets when using non-existent filter values
2. Error responses for invalid filter values

 def test_filter_by_status(self):
     status = "APPROVED"
     query = f"?status={status}"
     response = requests.get(
         f"{self.root_url}/api/v1/moderation-events/{query}",
         headers=self.basic_headers,
     )
     result = response.json()
     self.assertTrue(all(event['status'] == status for event in result['data']))
     self.assertEqual(response.status_code, 200)

+def test_filter_by_status_no_results(self):
+    status = "NON_EXISTENT_STATUS"
+    query = f"?status={status}"
+    response = requests.get(
+        f"{self.root_url}/api/v1/moderation-events/{query}",
+        headers=self.basic_headers,
+    )
+    result = response.json()
+    self.assertEqual(result['data'], [])
+    self.assertEqual(response.status_code, 200)

---

130-159: Enhance pagination test coverage

The pagination tests should also verify:

1. Consistent ordering across pages
2. No duplicate items between pages

 def test_search_after_pagination(self):
     # Step 1: Get the first set of results
     initial_size = 3
     query = f"?size={initial_size}&sort_by=created_at&order_by=asc"
     response = requests.get(
         f"{self.root_url}/api/v1/moderation-events/{query}",
         headers=self.basic_headers,
     )
     result = response.json()
     self.assertEqual(response.status_code, 200)
     self.assertEqual(len(result['data']), initial_size)
+    first_page_ids = {event['id'] for event in result['data']}

     # Step 2: Use the last item in the initial response for search_after
     last_event = result['data'][-1]
     search_after_value = last_event['created_at']
     search_after_query = f"?size={initial_size}&sort_by=created_at&order_by=asc&search_after={search_after_value}"

     # Step 3: Fetch the next page using search_after
     response = requests.get(
         f"{self.root_url}/api/v1/moderation-events/{search_after_query}",
         headers=self.basic_headers,
     )
     next_page_result = response.json()
     self.assertEqual(response.status_code, 200)
+    second_page_ids = {event['id'] for event in next_page_result['data']}
+    
+    # Verify no duplicate items between pages
+    self.assertEqual(len(first_page_ids.intersection(second_page_ids)), 0)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ad2e941 and 1f057e2.

📒 Files selected for processing (1)

• src/tests/v1/test_moderation_events.py (1 hunks)

🧰 Additional context used

📓 Learnings (1)

src/tests/v1/test_moderation_events.py (3)

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/tests/v1/test_moderation_events.py:16-17
Timestamp: 2024-11-13T07:11:20.995Z
Learning: In integration tests like `src/tests/v1/test_moderation_events.py` that run separately from the Django container, we can't import Django models like `ModerationEvent`.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/tests/v1/test_moderation_events.py:64-107
Timestamp: 2024-11-13T07:10:16.861Z
Learning: In integration tests located in `src/tests/v1`, we cannot import Django models like `ModerationEvent` because the tests run separately from the Django container.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/tests/v1/test_moderation_events.py:218-226
Timestamp: 2024-11-13T07:11:08.097Z
Learning: In integration tests, the `ModerationEvents` API is not accessible because it runs in a separate container.

🔇 Additional comments (3)

src/tests/v1/test_moderation_events.py (3)

1-7: LGTM!

The test class structure and imports are appropriate for integration testing.

---

1-243: Overall test coverage is comprehensive

The test suite provides thorough coverage of the moderation events API endpoint, including filtering, sorting, pagination, and validation scenarios. The integration tests are well-structured and follow best practices.

---

218-226: Verify the hardcoded moderation ID

The test uses a hardcoded UUID that might not exist in the test database. Consider:

1. Using a known valid UUID from test fixtures
2. Adding assertions to verify the returned data

✅ Verification successful

Hardcoded moderation ID is valid and exists in test fixtures

The UUID 3b50d60f-85b2-4a17-9f8d-5d3e1fc68198 is properly defined in src/django/api/fixtures/moderation_events.json, making it a reliable test case for validating moderation events.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the UUID exists in test fixtures
rg -g '*.json' '3b50d60f-85b2-4a17-9f8d-5d3e1fc68198' src/django/api/fixtures/

Length of output: 182

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)

src/tests/v1/test_moderation_events.py (5)

18-65: Consider documenting valid filter values in docstrings.

Since we can't import model constants in integration tests, it would be helpful to document the valid values for status, request_type, and source in the test class's docstring or individual test method docstrings.

---

122-132: Consider extracting max size limit as a constant.

The hardcoded value of 10000 for max_size could become outdated. Consider extracting it as a class constant for easier maintenance.

 class ModerationEventsTest(BaseAPITest):
+    MAX_SIZE_LIMIT = 10000
+
     def test_size_exceeds_max_limit(self):
-        max_size = 10000
+        max_size = self.MAX_SIZE_LIMIT

---

164-165: Consider using a more realistic invalid search_after value.

Using a future timestamp or ISO date string would be more realistic than a large number.

-        invalid_search_after_value = 999999999999999
+        invalid_search_after_value = '2025-12-31T23:59:59Z'

---

204-220: Enhance error response validation.

Consider adding assertions to validate the complete error response structure:

         self.assertEqual(response.status_code, 400)
         self.assertIn('errors', result)
         self.assertEqual(len(result['errors']), 1)
+        self.assertIsInstance(result['errors'], list)
 
         error = result['errors'][0]
+        self.assertIsInstance(error, dict)
+        self.assertCountEqual(['field', 'message'], error.keys())
         self.assertEqual(error['field'], 'Country')
         self.assertEqual(error['message'], "'Usa' Is Not A Valid Alpha-2 Country Code.")

---

231-246: Enhance error response validation for consistency.

Apply the same thorough error response structure validation as suggested for the country validation test.

         self.assertEqual(response.status_code, 400)
         self.assertIn('errors', result)
         self.assertEqual(len(result['errors']), 1)
+        self.assertIsInstance(result['errors'], list)
 
         error = result['errors'][0]
+        self.assertIsInstance(error, dict)
+        self.assertCountEqual(['field', 'message'], error.keys())
         self.assertEqual(error['field'], 'Moderation_Id')
         self.assertEqual(error['message'], "Invalid Uuid(S): 123!.")

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1f057e2 and aa8203b.

📒 Files selected for processing (1)

• src/tests/v1/test_moderation_events.py (1 hunks)

🧰 Additional context used

📓 Learnings (1)

src/tests/v1/test_moderation_events.py (3)

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/tests/v1/test_moderation_events.py:16-17
Timestamp: 2024-11-13T07:11:20.995Z
Learning: In integration tests like `src/tests/v1/test_moderation_events.py` that run separately from the Django container, we can't import Django models like `ModerationEvent`.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/tests/v1/test_moderation_events.py:64-107
Timestamp: 2024-11-13T07:10:16.861Z
Learning: In integration tests located in `src/tests/v1`, we cannot import Django models like `ModerationEvent` because the tests run separately from the Django container.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/tests/v1/test_moderation_events.py:218-226
Timestamp: 2024-11-13T07:11:08.097Z
Learning: In integration tests, the `ModerationEvents` API is not accessible because it runs in a separate container.

🔇 Additional comments (2)

src/tests/v1/test_moderation_events.py (2)

8-16: LGTM! Comprehensive basic endpoint test.

The test properly validates both the response status and structure.

---

67-109: LGTM! Sorting tests are comprehensive.

The tests adequately cover both ascending and descending sorting for created_at and status fields.

[roman-stolar]

Re-Approved.

[mazursasha1990]

Please pay attention to the opensearch query builder classes. Fix abstract methods and access modifiers for different methods.

Here is an article about Access Modifiers in Python : Public, Private and Protected

[coderabbitai]

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (11)

src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py (3)

1-4: Improve import readability

Consider reformatting the import statement for better readability:

-from api.views.v1.opensearch_query_builder. \
-    opensearch_query_builder import OpenSearchQueryBuilder
+from api.views.v1.opensearch_query_builder.opensearch_query_builder import (
+    OpenSearchQueryBuilder
+)

---

28-30: Consider using protected method in parent class

The current implementation uses name mangling to access the parent's private method. Consider making __build_os_id a protected method in the parent class to improve inheritance design.

# In parent class OpenSearchQueryBuilder:
-def __build_os_id(self, values):
+def _build_os_id(self, values):
    # implementation

# In this class:
-self._OpenSearchQueryBuilder__build_os_id(values)
+self._build_os_id(values)

---

44-49: Consider extracting string manipulation logic

The string manipulation for cleaned fields is repeated. Consider extracting this logic to a helper method:

+def _get_cleaned_field_path(self, field):
+    return 'cleaned_data.' + field[len('cleaned_'):]

 if (
     field == V1_PARAMETERS_LIST.CLEANED_NAME
     or field == V1_PARAMETERS_LIST.CLEANED_ADDRESS
 ):
     self.query_body['sort'].append({
-        'cleaned_data.' + field[len('cleaned_'):]: {'order': order_by}
+        self._get_cleaned_field_path(field): {'order': order_by}
     })

src/django/api/tests/test_moderation_events_query_builder.py (4)

3-4: Consider simplifying the import path

The long relative import path could be simplified by adding an intermediate __init__.py file in the opensearch_query_builder directory to allow direct imports.

-from api.views.v1.opensearch_query_builder. \
-    moderation_events_query_builder import ModerationEventsQueryBuilder
+from api.views.v1.opensearch_query_builder import ModerationEventsQueryBuilder

---

21-67: Consider adding edge cases to terms tests

While the current tests are thorough, consider adding test cases for:

• Single-value arrays
• Special characters in values
• Maximum length values

---

69-69: Consider making the date range builder method protected instead of private

The test uses name mangling to access a private method. Consider making __build_date_range a protected method (_build_date_range) since it needs to be tested directly.

---

99-106: Consider adding test with populated query body

While testing the default structure is good, consider adding a test case that verifies the final query structure with various query components (terms, range, sort, etc.) populated.

Example:

def test_get_final_query_body_with_components(self):
    self.builder.add_terms('country', ['US'])
    self.builder.add_sort('created_at', 'desc')
    final_query = self.builder.get_final_query_body()
    self.assertIn('terms', final_query['query']['bool']['must'][0])
    self.assertIn('sort', final_query)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (1)

Line range hint 47-60: Add input validation and documentation for OS ID builder.

The __build_os_id method should validate the input values and include documentation explaining the purpose of searching both fields.

def __build_os_id(self, values):
+    """
+    Build a query to search for OS IDs in both current and historical fields.
+    
+    Args:
+        values (list): List of OS IDs to search for
+    Raises:
+        ValueError: If values is empty or contains invalid IDs
+    """
+    if not values or not all(isinstance(v, str) for v in values):
+        raise ValueError("values must be a non-empty list of string IDs")
+
    self.query_body['query']['bool']['must'].append(
        {
            'bool': {
                'should': [
                    {'terms': {'os_id': values}},
                    {'terms': {'historical_os_id.keyword': values}},
                ]
            }
        }
    )

doc/release/RELEASE-NOTES.md (3)

13-14: Fix section header formatting

Remove trailing colons from section headers to comply with markdown style guidelines.

Apply this diff:

-### Database changes:
+### Database changes
 #### Migrations:
-### Scheme changes:
+### Scheme changes
-### Code/API changes:
+### Code/API changes
-### Architecture/Environment changes:
+### Architecture/Environment changes
-### Bugfix:
+### Bugfix
-### What's new:
+### What's new
-### Release instructions:
+### Release instructions

Also applies to: 17-17, 20-20, 23-23, 26-26, 29-29, 32-32

🧰 Tools

🪛 Markdownlint

14-14: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

---

32-35: Enhance release instructions with post-deployment verification steps

The release instructions should include steps to verify the deployment was successful, especially since this release includes database migrations and indexing.

Consider adding these verification steps:

 ### Release instructions
 * Ensure that the following commands are included in the `post_deployment` command:
     * `migrate`
+* After deployment, verify:
+    * Database migrations completed successfully
+    * Indexing completed without errors
+    * OpenSearch indexes are properly populated
+    * Logstash service is running and processing data

🧰 Tools

🪛 Markdownlint

34-34: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)

---

32-32: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

---

9-11: Add version control metadata

Consider adding Git tag/commit information to help track which code version corresponds to this release.

Add version control details:

 ## Introduction
 * Product name: Open Supply Hub
 * Release date: November 30, 2024
+* Git tag: v1.25.0
+* Git commit: <commit-hash>

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between aa8203b and 9adb9c9.

📒 Files selected for processing (7)

• doc/release/RELEASE-NOTES.md (1 hunks)
• src/django/api/tests/test_moderation_events_query_builder.py (1 hunks)
• src/django/api/tests/test_production_locations_query_builder.py (3 hunks)
• src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py (1 hunks)
• src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (4 hunks)
• src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py (2 hunks)
• src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• src/django/api/tests/test_production_locations_query_builder.py
• src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py
• src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py

🧰 Additional context used

📓 Learnings (1)

src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py (4)

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py:48-0
Timestamp: 2024-11-12T09:11:37.544Z
Learning: In the `ModerationEventsQueryBuilder` class (`src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py`), methods inherited from the parent class `OpenSearchQueryBuilder`, such as `_build_os_id`, are available and can be used without redefining them.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py:87-103
Timestamp: 2024-11-12T11:36:45.713Z
Learning: In the codebase, errors are not raised at the query builder level (e.g., in `ModerationEventsQueryBuilder`); error handling is managed elsewhere.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/tests/test_moderation_events_query_builder.py:86-97
Timestamp: 2024-11-13T07:59:00.032Z
Learning: In our codebase, parameters are validated at the model level before being passed to the OpenSearch builder methods. The builder methods assume inputs are valid and do not perform parameter validation or raise exceptions.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py:24-43
Timestamp: 2024-11-12T11:39:16.126Z
Learning: Terminology in the codebase is defined in the specifications and should not be renamed.

🪛 Markdownlint

doc/release/RELEASE-NOTES.md

34-34: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)

---

14-14: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

---

32-32: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

🔇 Additional comments (5)

src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py (3)

8-20: LGTM! Well-structured initialization

The constructor properly initializes the query builder with:

• Immutable default query body using deep copy
• Clear default sort configuration
• Extensible build options dictionary

---

21-23: LGTM! Clear and focused implementation

The private method correctly formats the country field path.

---

67-83: LGTM! Proper search_after implementation

The method correctly implements OpenSearch's search_after pagination:

• Lazily initializes search_after field
• Ensures sort criteria exists when using search_after
• Uses default sort as fallback

src/django/api/tests/test_moderation_events_query_builder.py (2)

12-20: LGTM! Basic query manipulation tests are well-structured

The tests for reset and size addition are clear, focused, and verify the expected behavior.

---

86-97: LGTM! Sort and search tests are comprehensive

The tests properly verify the sort order and search_after functionality, including the default sort order when search_after is used.

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (3)

39-68: Consider splitting range handling into separate methods

The method handles two distinct types of ranges (numeric and date), which could be separated for better maintainability and adherence to Single Responsibility Principle.

Consider refactoring like this:

def add_range(self, field, query_params):
-    if field in {
-        V1_PARAMETERS_LIST.NUMBER_OF_WORKERS,
-        V1_PARAMETERS_LIST.PERCENT_FEMALE_WORKERS
-    }:
-        min_value = query_params.get(f'{field}[min]')
-        max_value = query_params.get(f'{field}[max]')
-        # ... rest of numeric handling
-    elif field in {
-        V1_PARAMETERS_LIST.DATE_GTE,
-        V1_PARAMETERS_LIST.DATE_LT
-    }:
-        self.__build_date_range(query_params)
+    if self._is_numeric_range_field(field):
+        self._build_numeric_range(field, query_params)
+    elif self._is_date_range_field(field):
+        self._build_date_range(query_params)

+def _is_numeric_range_field(self, field):
+    return field in {
+        V1_PARAMETERS_LIST.NUMBER_OF_WORKERS,
+        V1_PARAMETERS_LIST.PERCENT_FEMALE_WORKERS
+    }

+def _is_date_range_field(self, field):
+    return field in {
+        V1_PARAMETERS_LIST.DATE_GTE,
+        V1_PARAMETERS_LIST.DATE_LT
+    }

+def _build_numeric_range(self, field, query_params):
+    min_value = query_params.get(f'{field}[min]')
+    max_value = query_params.get(f'{field}[max]')
+    # ... rest of numeric handling

---

70-88: Use single underscore for private methods

In Python, double underscores (name mangling) are rarely needed and can complicate inheritance. Single underscore is the conventional way to indicate private methods.

-    def __build_date_range(self, query_params):
+    def _build_date_range(self, query_params):

---

99-109: Add docstrings to abstract methods

Abstract methods define the interface contract. Adding docstrings would help implementers understand the expected behavior and parameters.

Example:

@abstractmethod
def add_sort(self, field, order_by=None):
    """
    Add sorting criteria to the query.

    Args:
        field: Field to sort by
        order_by: Sort direction ('asc' or 'desc')
    """
    pass

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c22e1fb and bd9b907.

📒 Files selected for processing (4)

• src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py (1 hunks)
• src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (3 hunks)
• src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder_interface.py (0 hunks)
• src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (1 hunks)

💤 Files with no reviewable changes (1)

• src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder_interface.py

🚧 Files skipped from review as they are similar to previous changes (2)

• src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py
• src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py

🧰 Additional context used

📓 Learnings (1)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (1)

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py:12-13
Timestamp: 2024-11-14T11:06:35.337Z
Learning: In `src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py`, input validation for methods like `add_from` is performed at a higher level, so additional validation within these methods is unnecessary.

🔇 Additional comments (2)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (2)

2-2: LGTM: Good use of abstract base class pattern

The transition to an abstract base class is appropriate for this query builder hierarchy, providing a clear contract for concrete implementations.

Also applies to: 6-6

---

10-11: LGTM: Clean and focused implementation

The method follows single responsibility principle, handling only the pagination offset setting.

[VadimKovalenkoSNF]

UPD: OpenSearch query builder inheritance has been refactored again. I removed OpensearchQueryBuilderInterface. Instead use ABC for class OpenSearchQueryBuilder(ABC) from abc module. Also, for sake of readability, I removed protected methods signatures at all. Current logic for query builders is:

OpenSearchQueryBuilder(ABC) -> ModerationEventsQueryBuilder | ProductionLocationsQueryBuilder.
Also, restructured methods order in OpenSearchQueryBuilder(ABC).

[roman-stolar]

LGTM

[mazursasha1990]

In general, looks good to me.
But I strongly recommend to escape from name-mangled method from a subclass and use protected (single underscore) method.
My thoughts I explained in the comment.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[mazursasha1990]

LGTM

[VadimKovalenkoSNF]

@mazursasha1990 I made _build_os_id() method as protected, pls re-check.

[roman-stolar]

Approved

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (2)

2-2: LGTM! Good architectural improvement.

Making OpenSearchQueryBuilder an abstract base class enforces a consistent interface across different query builders while allowing specialized implementations. This change improves the design by:

• Enforcing implementation of core query-building behavior
• Following the Interface Segregation Principle
• Providing a clear contract for subclasses

Also applies to: 6-6

---

39-69: Consider extracting numeric range logic to improve readability.

While the logic is correct, the method could be more maintainable by extracting the numeric range handling into a separate private method.

Consider this refactor:

    def add_range(self, field, query_params):
        if field in {
            V1_PARAMETERS_LIST.NUMBER_OF_WORKERS,
            V1_PARAMETERS_LIST.PERCENT_FEMALE_WORKERS
        }:
-            min_value = query_params.get(f'{field}[min]')
-            max_value = query_params.get(f'{field}[max]')
-
-            min_value = int(min_value) if min_value else None
-            max_value = int(max_value) if max_value else None
-
-            range_query = {}
-            if min_value is not None:
-                range_query['gte'] = min_value
-            if max_value is not None:
-                range_query['lte'] = max_value
-            if range_query:
-                build_action = self.build_options.get(field)
-
-                if build_action:
-                    build_action(field, range_query)
-                else:
-                    self.query_body['query']['bool']['must'].append({
-                        'range': {field: range_query}
-                    })
+            self.__build_numeric_range(field, query_params)
        elif field in {
            V1_PARAMETERS_LIST.DATE_GTE,
            V1_PARAMETERS_LIST.DATE_LT
        }:
            self.__build_date_range(query_params)

+    def __build_numeric_range(self, field, query_params):
+        min_value = query_params.get(f'{field}[min]')
+        max_value = query_params.get(f'{field}[max]')
+
+        min_value = int(min_value) if min_value else None
+        max_value = int(max_value) if max_value else None
+
+        range_query = {}
+        if min_value is not None:
+            range_query['gte'] = min_value
+        if max_value is not None:
+            range_query['lte'] = max_value
+
+        if range_query:
+            build_action = self.build_options.get(field)
+            if build_action:
+                build_action(field, range_query)
+            else:
+                self.query_body['query']['bool']['must'].append({
+                    'range': {field: range_query}
+                })

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bd9b907 and a8b5fe3.

📒 Files selected for processing (3)

• src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py (1 hunks)
• src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (3 hunks)
• src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/django/api/views/v1/opensearch_query_builder/moderation_events_query_builder.py
• src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py

🧰 Additional context used

📓 Learnings (1)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (2)

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py:12-13
Timestamp: 2024-11-14T11:06:35.337Z
Learning: In `src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py`, input validation for methods like `add_from` is performed at a higher level, so additional validation within these methods is unnecessary.

Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py:61-79
Timestamp: 2024-11-14T11:06:37.772Z
Learning: In the `OpenSearchQueryBuilder` class located at `src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py`, date validation is performed at a higher level in the application. Therefore, it's unnecessary to add additional date validation within the `__build_date_range` method.

🔇 Additional comments (4)

src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (4)

10-11: LGTM! Clean and focused implementation.

The method provides a clear interface for setting the pagination offset. Input validation is handled at a higher level, as confirmed by previous learnings.

---

70-88: LGTM! Clean date range implementation.

The method is focused and handles date range queries effectively. Date validation is appropriately handled at a higher level, as confirmed by previous learnings.

---

99-109: LGTM! Good use of abstract methods.

Making these methods abstract ensures that all query builders must implement core functionality while allowing for specialized implementations. This enforces a consistent interface across different query builders.

---

114-125: LGTM! Well-structured OS ID query implementation.

The method effectively handles searching across both current and historical OS IDs. The implementation is clear, focused, and follows best practices.