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

SALTO-5555: errors in fetch JSM forms #5873

Merged
merged 6 commits into from
May 12, 2024

Conversation

idoamit8
Copy link
Contributor

@idoamit8 idoamit8 commented May 8, 2024

In this PR I fixed the multiple errors in JSM Forms.


Additional context for reviewer

Currently, we are logging many errors related to failures in fetching JSM forms.

Was Now
Previously, when we failed to fetch forms for an entire project, we logged an error for each project in the account. Now, we log a single error for all projects.
When forms had missing titles, we logged an error for each form. Now, we log one error covering all forms and projects.

Example in oriSaltoTest:
image

In addition, I decreased the logs from Error to debug and added fetch warnings.

Important Note:

I will add tests after the initial review to ensure that my solution is acceptable.


Release Notes:
Jira adapter:

  • Optimized error logging for JSM form fetching to reduce noise: Now logging a single error per account for project-wide fetch failures and a unified error for all forms with missing titles.

User Notifications:
Jira adapter:

  • Optimized error logging for JSM form fetching to reduce noise: Now logging a single error per account for project-wide fetch failures and a unified error for all forms with missing titles.

Copy link
Contributor

@IdoZakk IdoZakk left a comment

Choose a reason for hiding this comment

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

⭐ ⭐ ⭐ ⭐ ⭐
Thank you for the very detailed commit comment. It made the review much easier.
I think the general concept is great, see my comments

packages/jira-adapter/src/filters/forms/forms.ts Outdated Show resolved Hide resolved
packages/jira-adapter/src/filters/forms/forms.ts Outdated Show resolved Hide resolved
@idoamit8 idoamit8 requested a review from IdoZakk May 9, 2024 06:40
@coveralls
Copy link

coveralls commented May 9, 2024

Coverage Status

coverage: 93.929%. remained the same
when pulling bed62e8 on idoamit8:SALTO-5555-ErrorsInFetchJsmForms
into 3a9dcaa on salto-io:main.

Copy link
Contributor

@IdoZakk IdoZakk left a comment

Choose a reason for hiding this comment

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

🚀
I am approving, please look at the comments and most importantly the one about different types of errors

packages/jira-adapter/src/filters/forms/forms.ts Outdated Show resolved Hide resolved
packages/jira-adapter/src/filters/forms/forms.ts Outdated Show resolved Hide resolved
packages/jira-adapter/src/filters/forms/forms.ts Outdated Show resolved Hide resolved
packages/jira-adapter/test/filters/forms/forms.test.ts Outdated Show resolved Hide resolved
@idoamit8 idoamit8 force-pushed the SALTO-5555-ErrorsInFetchJsmForms branch from 8a31e20 to 11eb971 Compare May 12, 2024 07:55
@tamtamirr tamtamirr enabled auto-merge (squash) May 12, 2024 09:34
@tamtamirr tamtamirr merged commit 7cd7b22 into salto-io:main May 12, 2024
18 of 19 checks passed
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.

None yet

5 participants