-
Notifications
You must be signed in to change notification settings - Fork 180
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
Handling copy failures and retry capability #4060
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the Dexie queries look like they should use other querying methods than just toArray
https://dexie.org/docs/Collection/Collection.first()
contentcuration/contentcuration/frontend/channelEdit/components/ContentNodeListItem/index.vue
Show resolved
Hide resolved
wait_for_status: true, | ||
}); | ||
}) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing is using the resulting promise from Promise.allSettled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Promise.allSettled
so that the rejections from the underlying promise doesn't result in console errors. Because here we don't need to handle rejected promises from copyContentNode
. Does it make sense sir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I use an empty catch here? (maybe a better alternative)
contentcuration/contentcuration/frontend/channelEdit/vuex/contentNode/actions.js
Show resolved
Hide resolved
@bjester closing this. Will re-open a fresh PR that will target hotfixes. Will keep in mind the issues raised here. |
Summary
This PR brings in the capability to retry failed copies and detection of failing copies.
Manual verification steps performed
To generate copy error state on the frontend, I raised an exception from the backend copy function. These are the checks I've performed:-
changesForSyncing
table are detected. (happens when copy fails due to a known reason)Screenshots (if applicable)
The below screenshot is a work in progress. I need to make some CSS changes to make the UI look as it is expected.
The below is the UI design that is expected.
Does this introduce any tech-debt items?
I tested the code thoroughly and I've the following concerns, they might be or might not be tech debts depending on how we treat them, but listing them out for clarity:-
Reviewer guidance
References
Closes #2850.
Closes #3914.
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)