Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Validate] Refactor some IF validations #4144

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

[Validate] Refactor some IF validations #4144

wants to merge 23 commits into from

Conversation

israelpoli
Copy link
Contributor

Related Issues

fixes:

Description

@israelpoli israelpoli requested review from YuvHayun and removed request for dantavori and ilaner March 31, 2024 09:24
YuvHayun
YuvHayun requested changes Mar 31, 2024
View reviewed changes
Copy link
Contributor

@YuvHayun YuvHayun left a comment

Choose a reason for hiding this comment

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

Looks good!
minor comments.

demisto_sdk/commands/validate/validators/IF_validators/IF115_unsearchable_key.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/validators/IF_validators/IF111_is_field_type_changed.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/validators/IF_validators/IF111_is_field_type_changed.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/validators/IF_validators/IF113_name_field_prefix.py Outdated Show resolved Hide resolved
demisto_sdk/commands/validate/tests/IF_validators_test.py
item_prefix: Optional[Union[List[str], str]], valid_prefix: str
):
"""
Given:
Copy link
Contributor

Choose a reason for hiding this comment

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

please explain the differences between the cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using pytest.param with id parameter instead docstring

dorschw
dorschw approved these changes Apr 1, 2024
View reviewed changes
demisto_sdk/commands/validate/tests/IF_validators_test.py
Comment on lines +218 to +225
# not valid
content_item = create_incident_field_object(["type"], ["html"])
old_content_items = [create_incident_field_object(["type"], ["short text"])]
create_old_file_pointers([content_item], old_content_items)
assert IsFieldTypeChangedValidator().is_valid([content_item])

# valid
content_item.field_type = "short text"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# not valid
content_item = create_incident_field_object(["type"], ["html"])
old_content_items = [create_incident_field_object(["type"], ["short text"])]
create_old_file_pointers([content_item], old_content_items)
assert IsFieldTypeChangedValidator().is_valid([content_item])
# valid
content_item.field_type = "short text"
old_type = "short text"
new_type = "html" # simulates modification
# not valid
content_item = create_incident_field_object(["type"], [new_type])
old_content_items = [create_incident_field_object(["type"], [old_type])]
create_old_file_pointers([content_item], old_content_items)
assert IsFieldTypeChangedValidator().is_valid([content_item])
# valid
content_item.field_type = old_type

demisto_sdk/commands/validate/tests/IF_validators_test.py
Comment on lines +263 to +275
with ChangeCWD(REPO.path):
content_item = create_incident_field_object(pack_info={"name": "Foo"})
results = NameFieldPrefixValidator().is_valid([content_item])
assert results
assert results[0].message == (
"Field name must start with the relevant pack name or one of the item prefixes found in pack metadata."
"\nFollowing prefixes are allowed for this IncidentField:"
"\nFoo"
)

# valid
content_item.name = "Foo CVE"
assert not NameFieldPrefixValidator().is_valid([content_item])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with ChangeCWD(REPO.path):
content_item = create_incident_field_object(pack_info={"name": "Foo"})
results = NameFieldPrefixValidator().is_valid([content_item])
assert results
assert results[0].message == (
"Field name must start with the relevant pack name or one of the item prefixes found in pack metadata."
"\nFollowing prefixes are allowed for this IncidentField:"
"\nFoo"
)
# valid
content_item.name = "Foo CVE"
assert not NameFieldPrefixValidator().is_valid([content_item])
pack_name = "Foo"
with ChangeCWD(REPO.path):
content_item = create_incident_field_object(pack_info={"name": pack_name})
results = NameFieldPrefixValidator().is_valid([content_item])
assert results
assert results[0].message == (
"Field name must start with the relevant pack name or one of the item prefixes found in pack metadata."
"\nFollowing prefixes are allowed for this IncidentField:"
f"\n{pack_name}"
)
# valid
content_item.name = "Foo CVE"
assert not NameFieldPrefixValidator().is_valid([content_item])

demisto_sdk/commands/validate/tests/IF_validators_test.py
def test_NameFieldPrefixValidator_is_valid_with_item_prefix(
item_prefix: Optional[Union[List[str], str]],
valid_prefix: str,
expected_allowed_prefixes: str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expected_allowed_prefixes: str,
expected_allowed_prefixes: List[str],

demisto_sdk/commands/validate/tests/IF_validators_test.py
Comment on lines +484 to +488
content_item = create_incident_field_object(["type"], ["html"])
old_content_items = [create_incident_field_object(["type"], ["short text"])]
create_old_file_pointers([content_item], old_content_items)
results = IsFieldTypeChangedValidator().fix(content_item)
assert content_item.field_type == "short text"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
content_item = create_incident_field_object(["type"], ["html"])
old_content_items = [create_incident_field_object(["type"], ["short text"])]
create_old_file_pointers([content_item], old_content_items)
results = IsFieldTypeChangedValidator().fix(content_item)
assert content_item.field_type == "short text"
old_type = "short text"
new_type = "html"
content_item = create_incident_field_object(["type"], [new_type])
old_content_items = [create_incident_field_object(["type"], [old_type])]
create_old_file_pointers([content_item], old_content_items)
results = IsFieldTypeChangedValidator().fix(content_item)
assert content_item.field_type == old_type

demisto_sdk/commands/validate/tests/IF_validators_test.py
create_old_file_pointers([content_item], old_content_items)
results = IsFieldTypeChangedValidator().fix(content_item)
assert content_item.field_type == "short text"
assert results.message == "Changed the `type` field back to `short text`."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert results.message == "Changed the `type` field back to `short text`."
assert results.message == f"Changed the `type` field back to `{old_type}`."

demisto_sdk/commands/validate/validators/IF_validators/IF113_name_field_prefix.py
Comment on lines +45 to +48
for prefix in allowed_prefixes(content_item):
if content_item.name.startswith(prefix):
return True
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for prefix in allowed_prefixes(content_item):
if content_item.name.startswith(prefix):
return True
return False
return any((content_item.name.startswith(prefix) for prefix in allowed_prefixes(content_item))

demisto_sdk/commands/validate/validators/IF_validators/IF113_name_field_prefix.py
]


def name_include_allowed_prefix(content_item: ContentTypes) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def name_include_allowed_prefix(content_item: ContentTypes) -> bool:
def is_name_using_allowed_prefix(content_item: ContentTypes) -> bool:

demisto_sdk/commands/validate/validators/IF_validators/IF113_name_field_prefix.py
Comment on lines +64 to +67
elif isinstance(item_prefix, list):
return prefixes | set(item_prefix)
else:
return prefixes | {item_prefix}
Copy link
Contributor

Choose a reason for hiding this comment

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

do from more_itertools import always_iterable, and then

Suggested change
elif isinstance(item_prefix, list):
return prefixes | set(item_prefix)
else:
return prefixes | {item_prefix}
return prefixes | set(always_iterable(item_prefix))

🪄

dorschw
dorschw requested changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@dorschw dorschw left a comment

Choose a reason for hiding this comment

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

Please link to a jira issue

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

@dorschw dorschw dorschw requested changes

@YuvHayun YuvHayun Awaiting requested review from YuvHayun

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@israelpoli @YuvHayun @dorschw