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

[IMP][12.0][base_exception] Improvements on base_exception #1609

Merged
merged 1 commit into from
Oct 11, 2019
Merged

[IMP][12.0][base_exception] Improvements on base_exception #1609

merged 1 commit into from
Oct 11, 2019

Conversation

ivantodorovich
Copy link
Contributor

@ivantodorovich ivantodorovich commented Jun 18, 2019 •
edited
Loading

This is what makes this possible in sale_exception (OCA/sale-workflow#878):

image

If modules like sale_exception implement this approach for displaying errors, main_exception_id field wouldn't be necessary anymore.

Also added copy=False to exception_ids because they should be reevaluated, not copied.

@ivantodorovich ivantodorovich mentioned this pull request Jun 18, 2019
Merged
1 task
hparfr
hparfr approved these changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@hparfr hparfr left a comment

Choose a reason for hiding this comment

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

  • main_exception can be still be good in tree view, where you don't have much space
  • copy=False is a good improvement.

@florian-dacosta
Copy link
Contributor

LGTM
But do not delete main_exception_id it still can be very usefull.
The exceptions have a sequence and sometimes you really just want to see the main exception, you can filter on this exception, group by this exception, etc...

@ivantodorovich
Copy link
Contributor Author

@florian-dacosta great!
Don't worry, not deleting anything here.
Anyways, if we were to delete something like this it shuold be done on the next migration, IMO.

rousseldenis
rousseldenis approved these changes Sep 23, 2019
View reviewed changes
Copy link
Sponsor Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review

@rafaelbn
Copy link
Member

rafaelbn commented Oct 1, 2019

Please @pedrobaeza @yajo or any @OCA/tools-maintainers could review here and merge? this PR is blocking other PR, thanks

yajo
yajo reviewed Oct 2, 2019
View reviewed changes
base_exception/models/base_exception.py Outdated Show resolved Hide resolved
base_exception/models/base_exception.py Outdated Show resolved Hide resolved
yajo
yajo requested changes Oct 7, 2019
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

This looks mostly fine now, thanks.

Just some whitespace to remove, as a matter of code convention.

Also please squash all commits in one, to do the final merge.

base_exception/models/base_exception.py Outdated Show resolved Hide resolved
base_exception/models/base_exception.py Outdated Show resolved Hide resolved
base_exception/models/base_exception.py Outdated Show resolved Hide resolved
@ivantodorovich
[IMP] Add convenience action to ignore exceptions on record
ee215ee 
[IMP] Computed exception descriptions field, to display better help messages

[IMP] Exceptions shouldn't be copied
@ivantodorovich
Copy link
Contributor Author

done @yajo !
Thanks for reviewing this :)

@yajo
Copy link
Member

yajo commented Oct 11, 2019

/ocabot merge

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-1609-by-Yajo-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Oct 11, 2019
@OCA-git-bot
Merge PR #1609 into 12.0
241ebb9 
Signed-off-by Yajo
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at fa199be. Thanks a lot for contributing to OCA. ❤️

@OCA-git-bot OCA-git-bot merged commit ee215ee into OCA:12.0 Oct 11, 2019
@ivantodorovich ivantodorovich deleted the 12.0-imp-base_exception branch October 14, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yajo yajo yajo approved these changes

@florian-dacosta florian-dacosta florian-dacosta approved these changes

@hparfr hparfr hparfr approved these changes

@rousseldenis rousseldenis rousseldenis approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ivantodorovich @florian-dacosta @rafaelbn @yajo @OCA-git-bot @hparfr @rousseldenis