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

Prevent users from losing work when they lose internet connection. #3295

Closed
Anthony-Alridge opened this issue Apr 6, 2017 · 13 comments
Closed
Assignees
Labels

Comments

@Anthony-Alridge
Copy link
Contributor

Anthony-Alridge commented Apr 6, 2017

A user may be in the process of creating an exploration when they lose internet connection. This can cause frustration as attempting to refresh the browser at this point will cause the user to lose any work which they have so far done on the page (e.g if they were inputting a large amount of text, it will be gone). See here for more detail.

The aim of this is to solve this frustration for the user by protecting their work when they suddenly go offline. A possible approach would be to detect when the user goes offline and at that point save the work they have completed so far into localStorage. This way after they refresh we still have the work saved locally. As a result it will be possible to repopulate the page with the work which they would have otherwise lost.

To replicate fire up the dev server and create an exploration. Click on a state to and add content to the state editor.
lotsoftext
Now disable the internet connection (check the offline radio button in the network tab in dev tools), then click save content. Oppia will prompt you to refresh.
oppiaaskstorefresh
So you fix your internet connection and refresh, but then the work is gone!
byebyework

This would clearly be frustrating for a user so we need a way to deal with it. Note that the problem isn't so much the loss of internet connection, but the fact that the user will lose any text they have written when they refresh if it is not yet saved. It would be nice to protect the user against this such that when they refresh without saving their work is still protected, preferably without needing any direct action on the user's part so that when they refresh they get...

whatwewant

@Anthony-Alridge
Copy link
Contributor Author

Hi @seanlip I've got a few questions about how to approach this.
I'm taking the problem to be that a creator who has entered a lot of work onto an exploration card will lose this work if they refresh their browser. (I think it's sufficient to focus on the situation where creator's are creating explorations, I can't think of anywhere else a user will enter enough text for this to be a frustration but correct me if I am wrong).

Now onto the solution, rather than saving the user's work when they go offline per se, I'm thinking it may be better to save the user's work (into localStorage) at regular intervals, then when the user refreshes the webpage we can restore they're work. This may be better because it solves the more general problem of the user losing work when they refresh the webpage, as opposed to just solving the issue of the user losing work when they go offline.

@seanlip
Copy link
Member

seanlip commented Apr 6, 2017

So, one thing I should mention is that we do already have functionality for autosaving drafts to the backend. I think localStorage could be part of a solution, but was wondering how the two things (drafts and localStorage) would interact with each other?

@Anthony-Alridge
Copy link
Contributor Author

Okay I've had a look at the code and from what I can tell it works as follows: whenever the user makes a change to the exploration (e.g. adding a new state or editing a state) they have to save it (e.g save content in the case of editing the content property of a state). This is registered as a change and is autosaved to the backend. The user manually saves by clicking save draft, which essentially commits the user to the changes he has made so far and clears the list of changes. Please correct me if I'm wrong about any of that.

Okay so onto the issue of integrating these functionalities. It seems to me that the only change which needs to saved to the frontend is the text content of a state editor (i.e the key could be the state name, the value would be the text content of the state editor), so we will not need to save the full changeList to the frontend. Furthermore, the way the changeList auto save behaves is quite different to the behavior we need here, hence the implementation of the local autosave functionality may be somewhat separate from the back end autosaves (e.g. we do not want to populate the list of changes every time the user types something into the state editor). The integration I think will play more of a role in terms of updating the autosaved data. That is, when the page is refreshed the autosaved data is fetched from the backend. It may be the case that we have a local change saved more recently that this backup local save and so we may need to update the state property of the autosaved change list with the more recent text content. A problem here would be in knowing exactly which is more recent (the local autosave or the backend autosave). Given how the backend autosave works however I think it will be reasonable to assume that the local autosave will be the more recent copy, so if there exists text content in the local autosave then we augment the autosave gathered from the backend to reflect the new content before returning the changelist. This could be done by looping through the keys (i..e state names) we've saved to localStorage and modifying the text content property of the appropriate state in the draft.

I think it makes sense to include these modifications in changeListService (where the autosave functionality is handled). To summarize, we need to expose one new function and modify one existing one. The new one should be capable of taking a state name and text content from a state editor and saving it to localStorage if a matching key:value pair (matching in the sense that the text contents are the same) doesn't exist. This could be called in intervals from StateEditor.js. Next we need to modify the loadAutoSaveData function which currently takes a draft and populates the list of changes in changeListService with this draft list. Instead it needs to first update the text contents of each state as necessary, and also return this newly updated draft to be used instead of the original draft fetched from the backend.

I may need to investigate a little more but does this seem like a viable approach to you?

@seanlip
Copy link
Member

seanlip commented Apr 9, 2017

Thanks, @Anthony-Alridge! Let's step back one second, first; I want to fully understand the exact "success criteria" for what solving this issue entails.

In the issue description at the top, would you mind providing some steps that one could take on a local server to simulate this, a description of what happens, and a description of what should instead happen? Note that there's a checkbox in the chrome devtools (Network tab) that you can use to simulate an offline connection.

In particular I want to understand the intended behaviour from the user perspective, and then we can drill down (based on your discussion above) to see what measures we need to take in order to bring about that behaviour.

Thanks!

@Anthony-Alridge
Copy link
Contributor Author

Thanks @seanlip I've amended it, how's that?

@seanlip
Copy link
Member

seanlip commented Apr 11, 2017

The new description is excellent. Thanks @Anthony-Alridge!

So, looking at your notes: I agree with your first paragraph. I think the components you are looking at are the right ones and I think the approach looks promising, but I would suggest thinking about two things:

  • how are you going to keep localStorage and the autosaved drafts in sync (eventually)? It seems to me that if you end up restoring from localStorage we should perhaps update the autosaved drafts as well?
  • would it make sense to store timestamps so that you can resolve which changes happened earlier/later?

Perhaps it might make sense to start a design doc in Google Docs so that you have more room to lay out your approach and so that it can be commented on.

Thanks!

@Anthony-Alridge
Copy link
Contributor Author

Hi @seanlip I've opened up a doc here. I need to add more detail into the exact changes (hopefully will do tomorrow) but it provides an overview similar to above, please tell me what you think. Thanks.

I've tried to address your points in the doc but indirectly so I'll comment here:

  • When loadAutoSaveData is called the changes will be amended by using the addChange helper method within ChangeListService, which will add these changes to the backend. This should keep them in sync.

  • I'm not sure if the timestamps will be necessary for this. I've explained it in the design doc but I think that the implementation will be such that the locally saved text content will always be more recent than any text content saved in the backend (provided that when the user clicks save content to save their draft of the text content the code is amended such that any local saves related to that state are deleted). I'm not against storing the timestamps of course but i'm not sure if it's necessary (and may it be worth avoiding having to change the models etc to reflect the change if not needed)?

Thanks!

@seanlip seanlip modified the milestone: Recommended projects Apr 14, 2017
@seanlip
Copy link
Member

seanlip commented Apr 15, 2017

Hi @Anthony-Alridge, I'm going to pass this to @wxyxinyu since I'm a bit strapped for bandwidth at the moment. She has a lot of experience with the saving system so she will be able to give you good feedback, probably this weekend. Sorry for the delay!

@wxyxinyu
Copy link
Contributor

Hi @Anthony-Alridge -- thanks for the doc! I left a few comments. I'm slightly confused by what exactly you plan to save in local storage, so some clarification would be nice.

@anmolshkl
Copy link
Contributor

@Anthony-Alridge ping?

@Anthony-Alridge
Copy link
Contributor Author

Hi sorry about the late response something came up last week, hey @wxyxinyu thanks for that. I've added some comments to the doc, does that clarify things?

@anthkris
Copy link
Contributor

@Anthony-Alridge @wxyxinyu A friendly ping. Any updates on this?

@tjiang11
Copy link
Contributor

@Anthony-Alridge @wxyxinyu ping

Anthony-Alridge pushed a commit to Anthony-Alridge/oppia that referenced this issue Jun 19, 2017
giritheja added a commit to giritheja/oppia that referenced this issue Jun 25, 2017
…date

* upstream/develop:
  SiteWide ACL: Milestone 1.2 (oppia#3517)
  Fix oppia#2394: added docstrings to core.domain.value_generators_domain (oppia#3564)
  Fix  oppia#3554: Migration of Fallbacks to Hints (oppia#3555)
  Fix oppia#3502: Extract components from HTML string (oppia#3549)
  Fix oppia#3295 Add backend support for draft change list id (oppia#3560)
  Fix oppia#3446: Numeric input validator (oppia#3470)
  Fetch all entities for the learner dashboard in a single RPC call. (oppia#3536)
  Improve image upload interface (step 1 of oppia#3505) (oppia#3552)
  Add a function for getting some sample answers for a given state. (oppia#3530)
  Add "back to collection" link to reduce potential confusion for users (oppia#3544)
  Fix a bug in the LogicProof interaction where operators were not being included in the student language, causing operators like 'A(x)' to be rejected even if they occur in the question. (oppia#3545)
  Fix oppia#3528 : Add helper functions for ClassifierTrainingJobModel (oppia#3529)
  Fix oppia#3438: Alternative to indicate next lesson in collection except Start here (oppia#3523)
seanlip pushed a commit that referenced this issue Jun 26, 2017
* add change list id field to user data model and update tests

* edit create or update draft to increment change list id(#3295)

* fix syntax error in test case

* fix lint error in test case

* Fix small spacing error

* remove trailing whitespace

* return draft id with exploration data and autosave put and add tests

* add draft id to exploration data factory method

* fix indentation error

* address review comments (#3565)

* fix indentation error

* Address review comments (#3565)

* Address review comments (#3565)
giritheja added a commit to giritheja/oppia that referenced this issue Jun 27, 2017
…date

* upstream/develop:
  Fix oppia#3557: make http://localhost:8181 open in default browser (oppia#3566)
  Allow user to resize images before uploading them in filepath editor (oppia#3574)
  Fix oppia#3295 Integrate draft change list id with front-end (oppia#3565)
  SiteWide ACL: Milestone 1.3 (oppia#3543)
@seanlip seanlip modified the milestone: Recommended projects Jul 20, 2017
tjiang11 pushed a commit that referenced this issue Sep 14, 2017
* add change list id field to user data model and update tests

* edit create or update draft to increment change list id(#3295)

* fix syntax error in test case

* fix lint error in test case

* Fix small spacing error

* remove trailing whitespace

* return draft id with exploration data and autosave put and add tests

* add draft id to exploration data factory method

* fix indentation error

* address review comments (#3565)

* fix indentation error

* Address review comments (#3565)

* Address review comments (#3565)

* Milestone 3 of extending autosave functionality

* remove unneeded comments

* remove tabs

* remove tabs

* check storage exists before removing item

* Introduced a local storage save service for exploration drafts

* Fix lint errors

* fix lint error

* Address review coments

* renaming local save service to local storage service.

* renaming localSaveService.js

* Milestone 4 completed

* Show lost changes modal when an invalid draft is in local storage

* offline autosave load is working and modal added.

* modify LocalStorageService tests to reflect new name

* Fix indentation errors

* Address review comments and add exploration draft object factory

* Add spec for draft object factory

* Load new object factory in exploration_editor.html

* fix linting errors

* fix linting errors

* fix typo

* rename method createFromDict to createFromJSON

* update tests to reflect new method names

* Address review comments

* Modify explorationData to use urlService.getPathname() to allow for easier mocking in unit tests

* Add tests for exploartion data getData to ensure localsave draft is correctly used.

* Fix lint errors

* Fix lint errors

* Fix lint errors

* remove console.log statement

* Addressing review comments.

* Address review comments.

* Fix lint errors

* Address review comments

* Add karma test for exploration draft object to local storage dict.

* Fix missing script load causing collection tests to fail

* Fix missing script load causing collection tests to fail
giritheja added a commit to giritheja/oppia that referenced this issue Sep 17, 2017
…t-m2

* upstream/develop: (27 commits)
  Fixes 3687: batch call for exploration and exploration rights object together. (oppia#3815)
  Fix part oppia#3400 Objective field directive (oppia#3740)
  Fix oppia#3800 Dropdown menu of the ABOUT in navigation bar (oppia#3855)
  Fix oppia#3295 Save draft change list to local storage. (oppia#3584)
  Disable learner dashboard feedback updates send button when no text entered. (oppia#3860)
  Disable save button while uploading audio to server; add Spanish support to audio languages; add audio loading message to learner view (oppia#3856)
  Fix oppia#3834: Introduce backend event models to record statistics (oppia#3841)
  Fix oppia#3852: Fix failing e2e test in develop (oppia#3853)
  Fix oppia#3826: Move validatorsService from app.js to ValidatorsService.js (oppia#3847)
  Update the max RAND limit for ID generation to fit within 32 bits. (oppia#3843)
  Fix oppia#3404: Add default placeholder text for mobile devices  (oppia#3845)
  Fix oppia#2553: Change the "index all explorations" job in the admin dashb… (oppia#3831)
  Add I18N to give up button; change tooltip text. (oppia#3844)
  Fix oppia#3721: Update the release_info script to use LCA and the most recent release tag in order to compile the list of changes, rather than relying on git describe. (oppia#3838)
  show_warning_only_on_button_click (oppia#3833)
  Deprecate splash page experiment (oppia#3829)
  Fixes for end to end working between Oppia and Oppia-ml. (oppia#3824)
  Fix 'Empty path passed in method' error on the collection page. (oppia#3827)
  Learner dashboard 3.2 (oppia#3759)
  📝 Fix oppia#3789 :Space out the profile drop down icons (oppia#3789). (oppia#3817)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants