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

send freshdesk api #1896

Merged
merged 9 commits into from
Jul 23, 2024
Merged

send freshdesk api #1896

merged 9 commits into from
Jul 23, 2024

Conversation

jzbahrai
Copy link
Contributor

@jzbahrai jzbahrai commented Jul 17, 2024

Summary | Résumé

If the template creation/ edit form has data in the Other field - We will send that data to freshdesk

Test instructions | Instructions pour tester la modification

Unfortunately we don't have a freshdesk integration for staging. The code is try/ except blocks for this exact reason.

Copy link

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

A question about the placement of the ticket creation in edit_service_template, and I think it would be good to update the tests to ensure send_new_template_category_request() is called as appropriate during template creation and template editing.

Comment on lines 940 to 958

# Send the information in form's template_category_other field to Freshdesk
# This code path is a little complex - We do not want to raise an error if the request to Freshdesk fails, only if template creation fails

if form.template_category_other.data:
is_english = get_current_locale(current_app) == "en"
try:
current_user.send_new_template_category_request(
current_user.id,
current_service.id,
form.template_category_other.data if is_english else None,
form.template_category_other.data if not is_english else None,
new_template.id,
)
except HTTPError as e:
current_app.logger.error(
f"Failed to send new template category request to Freshdesk: {e} for template {new_template.id}, data is {form.template_category_other.data}"
)

Copy link
Member

Choose a reason for hiding this comment

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

I think this code should be down near service_api_client.update_service_template() on line 984. As-is I think there is a chance this is going to send the request when a user is previewing their template.

Comment on lines 1018 to 1033
# Send the information in form's template_category_other field to Freshdesk
if form.template_category_other.data:
is_english = get_current_locale(current_app) == "en"
try:
current_user.send_new_template_category_request(
current_user.id,
current_service.id,
form.template_category_other.data if is_english else None,
form.template_category_other.data if not is_english else None,
new_template.id,
)
except HTTPError as e:
current_app.logger.error(
f"Failed to send new template category request to Freshdesk: {e} for template {new_template.id}, data is {form.template_category_other.data}"
)

Copy link
Member

Choose a reason for hiding this comment

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

Should this be here, again?

@jzbahrai jzbahrai force-pushed the task/add-freshdesk-cats branch from 4ad18c6 to 33ec66a Compare July 22, 2024 17:54
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally and behaves as expected!

@jzbahrai jzbahrai enabled auto-merge (squash) July 23, 2024 13:04
@jzbahrai jzbahrai merged commit 63b3cdb into main Jul 23, 2024
8 checks passed
@jzbahrai jzbahrai deleted the task/add-freshdesk-cats branch July 23, 2024 13:12
whabanks added a commit that referenced this pull request Jul 23, 2024
jzbahrai pushed a commit that referenced this pull request Jul 23, 2024
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.

2 participants