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

ELEMENTS-1281: handle errors on bulk action #345

Conversation

RSalem07
Copy link
Contributor

@RSalem07 RSalem07 commented Jan 4, 2021

I updated the requirements, This PR depends now on the platform one PR#4606

**Note for the reviewers ** :)

Initially I have used the error event name, Here But I got some errors on the unit tests, mainly on the nuxeo-audit-page-provider.test.js where the two tests below failed:

Should fire an event when an error occurs in the Operation call and Should return an error when occurs in the Operation call as they expect to get the notify event, but it failed. That's the main reason why I have changed the name to failure, before that I have tried different approche to try to fix it by keeping the error event name. Finally I used the failure let me know wdyt

thx

@RSalem07 RSalem07 requested a review from a team as a code owner January 4, 2021 15:10
@nuxeojenkins
Copy link
Contributor

View issue in JIRA: ELEMENTS-1281: Add Errors handler for Nuxeo Operation

core/nuxeo-operation.js Outdated Show resolved Hide resolved
@RSalem07 RSalem07 force-pushed the improvement-ELEMENTS-1281-handle-error-on-bulk-action-on-nuxeo-operation branch from 8a0a538 to 9e02258 Compare January 20, 2021 14:07
@RSalem07 RSalem07 force-pushed the improvement-ELEMENTS-1281-handle-error-on-bulk-action-on-nuxeo-operation branch from 9e02258 to 907d503 Compare January 26, 2021 11:49
@RSalem07 RSalem07 force-pushed the improvement-ELEMENTS-1281-handle-error-on-bulk-action-on-nuxeo-operation branch 2 times, most recently from 1e4b8f1 to 4b281c7 Compare January 28, 2021 17:56
core/nuxeo-operation.js Outdated Show resolved Hide resolved
andreacornaglia
andreacornaglia previously approved these changes Feb 2, 2021
core/nuxeo-operation.js Outdated Show resolved Hide resolved
core/nuxeo-operation.js Outdated Show resolved Hide resolved
@RSalem07 RSalem07 force-pushed the improvement-ELEMENTS-1281-handle-error-on-bulk-action-on-nuxeo-operation branch 2 times, most recently from d9a0050 to 5708e8b Compare February 9, 2021 10:34
@RSalem07
Copy link
Contributor Author

RSalem07 commented Feb 9, 2021

@Gabez0r as asked the WEB-UI pipeline with this Element branch is launched: https://qa.nuxeo.org/jenkins/job/master/job/web-ui-pipeline/349/

Copy link
Contributor

@richardsd richardsd left a comment

Choose a reason for hiding this comment

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

It looks good to me 👍

@nelsonsilva
Copy link
Contributor

thanks @RSalem07 clearly we have a lot of margin for improvement when it comes to error handling but I'm not a big fan of explicitly stopping error propagation on each element so I'd rather we spent some time figuring out a better, more holistic, approach to the problem.
The operation execution does throw error when called programatically, so you can get the error in the catch block. Also we set an error property which you could listen too if you really wanted with on-error-changed so I'm not sure how much of this you really need. Do you have a PR with the consumer of this improvements (ie the bulk action where you'd like to report errors) ?

@RSalem07
Copy link
Contributor Author

Hey @nelsonsilva :)

For now the one that needs this improvement is mainly the retention add-on. I have opened a PR nuxeo/nuxeo-retention#42

Also we set an error property which you could listen too if you really wanted with on-error-changed so I'm not sure how much of this you really need

ah I see your point, my initial idea was to keep the same logic then the others (start pooling...)

thx

@RSalem07 RSalem07 force-pushed the improvement-ELEMENTS-1281-handle-error-on-bulk-action-on-nuxeo-operation branch from 5708e8b to ffa6848 Compare March 5, 2021 11:10
@RSalem07
Copy link
Contributor Author

RSalem07 commented Mar 5, 2021

@nelsonsilva following our discussion, I have reverted the stop propagation error. I kept the update for a better error handling. please note that this PR is related to the backend one PR#4606. I have also update the retention one to use a simple catch on the promise PR#42.

Thx,

@RSalem07
Copy link
Contributor Author

RSalem07 commented Mar 5, 2021

new Jenkins build on WEB-UI https://qa.nuxeo.org/jenkins/job/master/job/web-ui-pipeline/354/ -> OK

@RSalem07 RSalem07 merged commit a8b3aa6 into master Apr 1, 2021
@RSalem07 RSalem07 deleted the improvement-ELEMENTS-1281-handle-error-on-bulk-action-on-nuxeo-operation branch April 1, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants