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

T1894 improve error messages on scan import letters #1986

Open
wants to merge 18 commits into
base: 14.0
Choose a base branch
from

Conversation

jordyBSK
Copy link
Contributor

improve error messages on scan import letters

Copy link

sonarcloud bot commented Nov 14, 2024

Comment on lines +309 to +313
self.import_completed = False
self.failed_file_name = file_name
self.state = "failed"
self.env.user.notify_danger(f"Couldn't import file {file_name}")
self._compute_state = False
Copy link
Member

Choose a reason for hiding this comment

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

A call to the write method is preferable when changing many values at the same time. Also assigning the compute method seems wrong.

Suggested change
self.import_completed = False
self.failed_file_name = file_name
self.state = "failed"
self.env.user.notify_danger(f"Couldn't import file {file_name}")
self._compute_state = False
self.write({
"import_completed": False,
"failed_file_name": file_name,
"state": "failed"
})
self.env.user.notify_danger(f"Couldn't import file {file_name}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I remove the line "self._compute_state = False" the import is recalculated and the import state is set to ready instead of failed

Copy link
Member

Choose a reason for hiding this comment

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

Can you then please put this as a comment above the line in order to explain the reason of this line? Otherwise won't understand and may remove it in the future.

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