Skip to content

[OSDEV-1346] Remove null properties from OpenSearch output#417

Merged
VadimKovalenkoSNF merged 14 commits intomainfrom
OSDEV-1346-remove-empty-values-from-the-opensearch-response
Nov 28, 2024
Merged

[OSDEV-1346] Remove null properties from OpenSearch output#417
VadimKovalenkoSNF merged 14 commits intomainfrom
OSDEV-1346-remove-empty-values-from-the-opensearch-response

Conversation

@VadimKovalenkoSNF
Copy link
Contributor

@VadimKovalenkoSNF VadimKovalenkoSNF commented Nov 18, 2024

Follow-up PR to OSDEV-1346.

Disable null values from the response of the OpenSearch.
Disable possible null os_id, claim_id and source from PATCH response.

@barecheck
Copy link

barecheck bot commented Nov 18, 2024

React App | Jest test suite - Code coverage report

Total: 27.8%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/django/api/serializers/v1/opensearch_common_validators/search_after_validator.py (2)

1-5: Add type hints and documentation

Consider adding:

  1. Type hint for the data parameter in the interface method
  2. Class-level docstring explaining the validator's purpose and expected input format
 from typing import List
 from api.serializers.v1.opensearch_validation_interface \
     import OpenSearchValidationInterface
 
 class SearchAfterValidator(OpenSearchValidationInterface):
+    """
+    Validates the search_after parameter in OpenSearch queries.
+    The search_after parameter must contain exactly two comma-separated values.
+    """

1-23: Consider adding unit tests and documentation

As this validator is part of the OpenSearch integration:

  1. Add comprehensive unit tests covering various input scenarios
  2. Document the expected format of search_after values in the API documentation
  3. Consider adding examples in the docstring showing valid and invalid inputs

Would you like me to help generate a test suite for this validator?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8a1572b and a19f354.

📒 Files selected for processing (2)
  • src/django/api/serializers/v1/moderation_event_update_serializer.py (1 hunks)
  • src/django/api/serializers/v1/opensearch_common_validators/search_after_validator.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/django/api/serializers/v1/moderation_event_update_serializer.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
doc/release/RELEASE-NOTES.md (2)

23-24: Enhance the OpenSearch null value handling description

The current description "Disabled null values from the response of the OpenSearch" is too vague. Consider providing more details about:

  • Which specific fields are affected
  • The rationale behind this change
  • Any potential impact on API consumers
-* [OSDEV-1346](https://opensupplyhub.atlassian.net/browse/OSDEV-1346) - Create GET request for `v1/moderation-events` endpoint. Disabled null values from the response of the OpenSearch. Disabled possible null `os_id`, `claim_id` and `source` from PATCH response.
+* [OSDEV-1346](https://opensupplyhub.atlassian.net/browse/OSDEV-1346) - Create GET request for `v1/moderation-events` endpoint. Enhanced OpenSearch response by removing null values to improve data quality and reduce payload size. Specifically removed null values for `os_id`, `claim_id` and `source` fields from PATCH response to ensure consistent data representation.

Line range hint 8-10: Document migration impact and rollback procedures

The database migrations section should include:

  • Impact assessment of each migration
  • Any required downtime
  • Rollback procedures if needed
#### Migrations:
-* 0159_alter_status_of_moderation_events_table.py - This migration alters status of api_moderationevent table.
-* 0160_allow_null_parsing_errors_in_facilitylist.py - This migration allows empty parsing_errors in api_facilitylist.
+#### Migrations:
+* 0159_alter_status_of_moderation_events_table.py
+  - Impact: Modifies status options in api_moderationevent table
+  - Downtime: None required
+  - Rollback: Revertible via standard Django migration
+
+* 0160_allow_null_parsing_errors_in_facilitylist.py
+  - Impact: Enables empty parsing_errors in api_facilitylist
+  - Downtime: None required
+  - Rollback: Revertible via standard Django migration
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a19f354 and 053ead4.

📒 Files selected for processing (1)
  • doc/release/RELEASE-NOTES.md (1 hunks)

roman-stolar
roman-stolar previously approved these changes Nov 22, 2024
Copy link
Contributor

@roman-stolar roman-stolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mazursasha1990
mazursasha1990 previously approved these changes Nov 28, 2024
Copy link
Contributor

@mazursasha1990 mazursasha1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

vladsha-dev
vladsha-dev previously approved these changes Nov 28, 2024
Copy link
Contributor

@vladsha-dev vladsha-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left optional comment
But LGTM

Copy link
Contributor

@vladsha-dev vladsha-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
LGTM

Copy link
Contributor

@mazursasha1990 mazursasha1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants