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.

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

Refactor/date limit soc feedback command #38769

Open
wants to merge 9 commits into
base: contrib/hoxhunt_refactor/date-limit-soc-feedback-command
Choose a base branch
from

Conversation

tomaskukk
Copy link
Contributor

@tomaskukk tomaskukk commented Feb 26, 2025

Contributing to Cortex XSOAR Content

Make sure to register your contribution by filling the contribution registration form

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

fixes: No issue

Description

Added date parsing for the threat_feedback_reported_at_limit field which is used as an argument in a command. This allows users to pass the value in user-friendly formats.

Must have

  • Tests
  • Documentation

@content-bot content-bot added Contribution Thank you! Contributions are always welcome! External PR Partner Support Level Indicates that the contribution is for Partner supported pack labels Feb 26, 2025
@content-bot content-bot changed the base branch from master to contrib/hoxhunt_refactor/date-limit-soc-feedback-command February 26, 2025 08:37
@content-bot
Copy link
Collaborator

Thank you for your contribution. Your generosity and caring are unrivaled! Make sure to register your contribution by filling the Contribution Registration form, so our content wizard @sapirshuker will know the proposed changes are ready to be reviewed.
For your convenience, here is a link to the contributions SLAs document.

@content-bot
Copy link
Collaborator

Hi @tomaskukk, thanks for contributing to the XSOAR marketplace. To receive credit for your generous contribution please follow this link.

Copy link
Contributor

@sapirshuker sapirshuker left a comment

Choose a reason for hiding this comment

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

Hi @tomaskukk ,
Thank you for your contribution!
Good work :)
Please see my suggestions.
In addition please fix the pre-commit issues.
Please feel free to reach out to me with any questions - I'm available here or on slack :)
Thanks again


##### Hoxhunt v2

- Added support for more flexible date formats in the `threat_feedback_reported_at_limit` argument.
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
- Added support for more flexible date formats in the `threat_feedback_reported_at_limit` argument.
- Added support for more flexible date formats in the *threat_feedback_reported_at_limit* argument in the ***hoxhunt-incident-soc-feedback-send*** command.

@@ -457,6 +457,10 @@ def camel_to_title(text: str) -> str:
result.append(char)
return ''.join(result).strip().title()

# Hoxhunt GQL API expects dates in ISO format
def format_date_to_iso_string(date: datetime | None) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

You have the same functionality in the CommonServerPython timestamp_to_datestring and date_to_timestamp
These methods already exist, you can make use of them.

Copy link
Contributor Author

@tomaskukk tomaskukk Feb 27, 2025

Choose a reason for hiding this comment

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

The date_to_timestamp returns the date as int (so probably epoch unix timestamp), and timestamp_to_datestring takes in the date as int or str, so I would need to parse the datetime to int / str first.

Would using the built in strftime be okay here so no need to do extra conversions?

Copy link
Contributor

@sapirshuker sapirshuker Mar 2, 2025

Choose a reason for hiding this comment

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

According to the docs, timestamp_to_datestring parse timestamp (milliseconds) to a date string and takes str or int, str such as "1541494441222" and int such as 1541494441222
So you need to use date_to_timestamp in order to convert the date to milliseconds and timestamp_to_datestring in order to convert the milliseconds to iso format. Another way is to use date.isoformat()

@sapirshuker sapirshuker added the pending-contributor The PR is pending the response of its creator label Feb 27, 2025
@tomaskukk
Copy link
Contributor Author

Hey @sapirshuker, I fixed the documentation suggestion and commented on the other comment. Pre-commit is passing now (the validation passed locally without issues but raised issues on CI, even with newest demisto-sdk version 🤔).

Should be good to be reviewed again!

@sapirshuker sapirshuker added the ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. label Mar 2, 2025
@content-bot
Copy link
Collaborator

For the Reviewer: Trigger build request has been accepted for this contribution PR.

@content-bot
Copy link
Collaborator

For the Reviewer: Successfully created a pipeline in GitLab with url: https://gitlab.xdr.pan.local/xdr/cortex-content/content/-/pipelines/2604160

@content-bot content-bot removed the ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. label Mar 2, 2025
@@ -232,3 +232,6 @@ tests:
defaultclassifier: Hoxhunt-classifier
defaultmapperin: Hoxhunt-mapper-incoming
defaultmapperout: Hoxhunt-mapper-outgoing
sectionOrder:
- Connect
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the section header to the to the beginning of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! (this was formatting work of demisto-sdk format)

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2025

CLA assistant check
All committers have signed the CLA.

@tomaskukk tomaskukk requested a review from sapirshuker March 3, 2025 06:33
Copy link
Contributor

@sapirshuker sapirshuker left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your contribution! Did you notice the comment I wrote in response to your question? If not, I will write it again regarding the use of existing functions to convert the date to the requested format. Thank you! Waiting for your response.



def format_date_to_iso_string(date: datetime | None) -> str:
return date.strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z' # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the docs, timestamp_to_datestring parse timestamp (milliseconds) to a date string and takes str or int, str such as "1541494441222" and int such as 1541494441222
So you need to use date_to_timestamp in order to convert the date to milliseconds and timestamp_to_datestring in order to convert the milliseconds to iso format. Another way is to use date.isoformat()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this comment completely earlier. I will fix it now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, used the date.isoformat!

@tomaskukk tomaskukk requested a review from sapirshuker March 3, 2025 13:30
@sapirshuker sapirshuker added the ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. label Mar 3, 2025
@content-bot
Copy link
Collaborator

For the Reviewer: Trigger build request has been accepted for this contribution PR.

@content-bot
Copy link
Collaborator

For the Reviewer: Successfully created a pipeline in GitLab with url: https://gitlab.xdr.pan.local/xdr/cortex-content/content/-/pipelines/2625584

@content-bot
Copy link
Collaborator

For the Reviewer: Trigger build request has been accepted for this contribution PR.

@content-bot content-bot removed the ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. label Mar 3, 2025
@content-bot
Copy link
Collaborator

For the Reviewer: Successfully created a pipeline in GitLab with url: https://gitlab.xdr.pan.local/xdr/cortex-content/content/-/pipelines/2625588

Copy link
Contributor

@sapirshuker sapirshuker left a comment

Choose a reason for hiding this comment

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

Hi @tomaskukk , thank you very much for the change, the contribution, and your patience. Working with dates can be frustrating and there are edge cases that need to be considered. While the contribution is short, we need to ensure that we are not causing bugs. We can schedule a meeting together to think about the solution together.

@@ -153,7 +156,7 @@ script:
type: unknown
- name: threat_feedback_reported_at_limit
required: true
description: Datetime limit as a iso string (e.g. "2024-10-30T08:37:42.359Z").
description: Datetime limit. Accepts (<number> <time unit>), e.g "7 days", "one month" or a iso string (e.g. "2024-10-30T08:37:42.359Z").
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add this change to the readme file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

@@ -639,7 +645,7 @@ def get_modified_remote_data_command(client: Client, args: dict, params: dict):
remote_args = GetModifiedRemoteDataArgs(args)
last_update = remote_args.last_update
last_update_parsed = dateparser.parse(last_update, settings={'TIMEZONE': 'UTC'})
last_update_utc = last_update_parsed.strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z' # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's only change what's necessary for the command you want to modify (hoxhunt-incident-soc-feedback-send) to avoid from unwanted impacting on users.

@@ -577,7 +583,7 @@ def get_remote_data_command(client: Client, args: dict, params: dict):
incident_id = parsed_args.remote_incident_id
last_update = parsed_args.last_update
last_update_parsed = dateparser.parse(last_update, settings={'TIMEZONE': 'UTC'})
last_update_utc = last_update_parsed.strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z' # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's only change what's necessary for the command you want to modify (hoxhunt-incident-soc-feedback-send) to avoid from unwanted impacting on users.

@@ -502,7 +508,7 @@ def fetch_incidents(

max_fetch_parsed = min(int(max_fetch), 100)

timefrom = first_fetch_parsed.strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's only change what's necessary for the command you want to modify (hoxhunt-incident-soc-feedback-send) to avoid from unwanted impacting on users.

@@ -742,7 +748,10 @@ def hoxhunt_send_incident_soc_feedback_command(client: Client, args: dict, param
incident_id = args.get('incident_id')
custom_message = args.get('custom_message')
threat_feedback_reported_at_limit = args.get('threat_feedback_reported_at_limit')
response = client.send_incident_soc_feedback(incident_id, custom_message, threat_feedback_reported_at_limit)
threat_feedback_reported_at_limit_parsed = dateparser.parse(threat_feedback_reported_at_limit) # type: ignore
date_limit = format_date_to_iso_string(threat_feedback_reported_at_limit_parsed)
Copy link
Contributor

@sapirshuker sapirshuker Mar 4, 2025

Choose a reason for hiding this comment

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

I tested your use case with the following code:

date_utc=dateparser.parse("2024-10-30T08:37:42.359Z") (as mentioned in the yml)
print(date_utc) >> 2024-10-30 08:37:42.359000+00:00
print(date_utc.isoformat(timespec='milliseconds')) >> 2024-10-30T08:37:42.359+00:00
print(date_utc.isoformat(timespec='milliseconds')+'Z') >> 2024-10-30T08:37:42.359+00:00Z

I don't think this is the wanted result, Please let me know if I'm wrong.
In addition when executing:
date_utc=dateparser.parse("7 days")
I get the result according to my timezone and you add "Z" to the output from "format_date_to_iso_string"
method meaning UTC so the the conversion is incorrect (What was also present before the change you made in the function format_date_to_iso_string in order to use the isoformat function).
Maybe let's figure out together what the use case and what we should do.
We can try and see if using the function arg_to_datetime can solve the problem for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, it is not indeed the result we want. Seems like the unit tests didn't catch this, which I was relying in. I wonder if it would just be safest to use the custom formatting function that we already had in place, as in the original version of the pull request, as it's proven to work? Is there a specific reason we can't use that, which is proven at this point.

I did not go with the date_to_timestamp and timestamp_to_datestring functions because they lost milliseconds in the process.

Copy link
Contributor

@sapirshuker sapirshuker Mar 4, 2025

Choose a reason for hiding this comment

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

@tomaskukk
I saw your original solution but as I wrote, when you are executing:
date_utc=dateparser.parse("7 days")
You get the time in the current time zone, for example my timezone is GMT+2 the time now is 2025-03-05 13:41:10.382902
When executing date_utc=dateparser.parse("7 days") we get 2025-02-25 13:41:10.382902
Then print(date_utc.strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z') as your original version you get >> 2025-02-25T13:41:10.382Z
mean the same time in UTC, I don't think this is the wanted result, Please let me know if I'm wrong.

date_7_days=dateparser.parse("7 Days") print(date_7_days) >>2025-02-25 13:41:10.382902 print(date_7_days.strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z') >> 2025-02-25T13:41:10.382Z

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @tomaskukk,
We have additional function from common server python named arg_to_datetime
Please check if this meets your need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I didn't realize in the beginning that the motivation of the comment was the timezone related issue 👍

I'll see and fix it (alongside the other comments)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Form Filled Whether contribution form filled or not. Contribution Thank you! Contributions are always welcome! External PR Partner Support Level Indicates that the contribution is for Partner supported pack Partner Partner-Approved pending-contributor The PR is pending the response of its creator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants