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

Improve facility sync status reporting to users #9541

Merged

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jun 28, 2022

Summary

Previously, it wasn't possible to find out whether the last sync was successful or if it failed from the facility object itself. We were only able to derive information about failure from task objects while a task was running and that information wasn't persisted on the page reload, resulting in confusing UX.

This PR improves facility sync status reporting to users by persisting information about the success or failure of a facility sync task on the facility object by replacing the last_synced field by last_successful_sync and last_failed_sync fields (that said, we still continue using the failed task information from the task object to display fast update on the sync process to users without the need to re-fetch a facility when the task fails)

Preview of possible states

Never synced There was no successful sync in the past and the most recent sync failed There was a successful sync in the past and the most recent sync failed The most recent sync was successful
never-synced sync-failed success-fail success

References

Resolves #9091

Reviewer guidance

  • From "Device -> Facilities" or from "Facility -> Data", sync a facility to another facility or to KDP to test successful sync information
  • We currently have no simple mechanism for end-to-end testing of a sync failure - we're planning to do it at least once together with @bjester though

Testing checklist

  • 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
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • 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

@MisRob MisRob force-pushed the facility-sync-error-message branch 2 times, most recently from 3762195 to 5de8407 Compare June 30, 2022 14:25
Differentiate between successful and failed sync

Resolves learningequality#9091
@MisRob MisRob force-pushed the facility-sync-error-message branch from 5de8407 to 58e9582 Compare June 30, 2022 15:34
@MisRob MisRob changed the title [WIP] More granular facility sync status information Improve facility sync status information Jul 1, 2022
@MisRob MisRob changed the title Improve facility sync status information Improve facility sync status reporting to users Jul 1, 2022
@MisRob MisRob added this to the Planned Patch 4: Coach usability improvements milestone Jul 1, 2022
@MisRob MisRob requested a review from bjester July 1, 2022 12:32
@MisRob MisRob added TODO: needs review Waiting for review DEV: backend Python, databases, networking, filesystem... DEV: frontend labels Jul 1, 2022
@MisRob MisRob marked this pull request as ready for review July 1, 2022 12:33
@MisRob MisRob requested a review from jredrejo July 1, 2022 12:36
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Look good to me, great work. I only have left a couple of comments with the same doubt about a filter, if we can clarify it, it'd be ready from my pov

@MisRob
Copy link
Member Author

MisRob commented Jul 7, 2022

Thank you @jredrejo, I've just replied

@MisRob MisRob requested a review from jredrejo July 8, 2022 08:19
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.

💯

@MisRob
Copy link
Member Author

MisRob commented Jul 8, 2022

We've just tested syncing failure and success with @bjester and looks to be good

kolibri/core/auth/api.py Show resolved Hide resolved
@MisRob MisRob merged commit f05e690 into learningequality:release-v0.15.x Jul 9, 2022
@MisRob MisRob deleted the facility-sync-error-message branch February 17, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... DEV: frontend TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants