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

Allow users to retry copy failures #4176

Merged
merged 8 commits into from
Sep 5, 2023

Conversation

vkWeb
Copy link
Member

@vkWeb vkWeb commented Jun 23, 2023

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:-

  1. Checked whether error in the changesForSyncing table are detected.
  2. Checked whether importing from channels errors out.
  3. Checked whether copying completes.
  4. Checked whether retrying completes successfully when the exception is removed from the backend code.
  5. Checked whether after multiple failures, retrying still reaches success upon exception removal.
  6. Copying multiple nodes together by selecting them works as expected.
  7. When copy is failed, the node can be removed.

Screenshots (if applicable)

The below screenshot is the current design, it's matching with the expected design. (thanks to sir @bjester for helping me out fix the design).

image

The below is the UI design that is expected.

image

Does this introduce any tech-debt items?

Possible tech-debts:-

  • the CSS that is used to prevent wrapping retry action can be refactored to make it responsive and robust.
  • due to the current architecture, copy error state doesn't persist across sessions.

Reviewer guidance

Does manual verification steps checks out green?

References

Closes #2850.
Closes #3914.


Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • If this includes an internal dependency change, a link to the diff is provided
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?
  • If any Python requirements have changed, the updated requirements.txt files also included in this PR
  • Opportunities for using Google Analytics here are noted
  • Migrations are safe for a large db

Studio-specifc:

  • All user-facing strings are translated properly
  • The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs
  • Users' storage used is recalculated properly on any changes to main tree files
  • If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@vkWeb vkWeb requested review from bjester and rtibbles June 23, 2023 21:16
@vkWeb
Copy link
Member Author

vkWeb commented Jul 6, 2023

@bjester I've fixed the merge conflict, ready for review & merge.

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Some questions, but only two main areas to focus on for requested for changes:

  1. Seems undoing a copy isn't working-- there was an issue that was fixed in hotfixes but since you merged that in, I expect it to still work
  2. Some CSS issues-- the copy failure text and CTA is causing overflow for smaller widths, and the CTA and message are different font sizes. Additionally, it could use better vertical alignment between the icon, text and CTA

Screenshot from 2023-07-06 11-53-46
Screenshot from 2023-07-06 11-53-19
Screenshot from 2023-07-06 11-52-54

@vkWeb
Copy link
Member Author

vkWeb commented Jul 7, 2023

Seems undoing a copy isn't working-- there was an issue that was fixed in hotfixes but since you merged that in, I expect it to still work

I tried on hotfixes and unstable, I am not able to undo copying. I think we need to fix it, let's do it with this PR itself @bjester?

Some CSS issues-- the copy failure text and CTA is causing overflow for smaller widths, and the CTA and message are different font sizes. Additionally, it could use better vertical alignment between the icon, text and CTA

Let me try sir fixing those!

@vkWeb
Copy link
Member Author

vkWeb commented Jul 7, 2023

I checked why undoing copy isn't working. Here's my take sir: the record is being successfully deleted only at the indexed db level, Vuex isn't reflecting it, backend isn't affected as well. Record still exists on the backend.

@bjester
Copy link
Member

bjester commented Jul 7, 2023

You're right-- the UNDO of a copy seems to have regressed again

@vkWeb
Copy link
Member Author

vkWeb commented Jul 14, 2023

@bjester I've fixed the font-size. Fixing font-size seemed to have solved the vertical-alignment problem I feel. So, this is ready for final review and merge sir 🎉 Let me know if it needs anything!

@vkWeb
Copy link
Member Author

vkWeb commented Jul 14, 2023

While manual testing this, I have observed an edge case. Putting this as draft until I solve it.

@vkWeb vkWeb marked this pull request as draft July 14, 2023 13:06
@bjester bjester marked this pull request as ready for review July 17, 2023 15:10
@bjester bjester changed the title Copy fail handle Allow users to retry copy failures Jul 17, 2023
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

@vkWeb There's still some misalignment with the text. I committed one change to your PR, on code that was pre-existing. Let me know if you have questions about that.
screenshot_2023-07-17_at_1 35 40_pm_720

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Created a new issue for the misalignment of the CTA as discussed. Merging this!

@bjester bjester merged commit 7814874 into learningequality:hotfixes Sep 5, 2023
@bjester bjester mentioned this pull request Sep 27, 2023
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