search

mdenet / educationplatform

Eclipse Public License 2.0
2 stars 3 forks source link

Second save fails with error message #139

Open barnettwilliam opened 8 months ago

barnettwilliam commented 8 months ago

When a user clicks the save button for a second time an error is reported by a red popover box. It appears to be for the panel that the change was made. Seen during the MDNet symposium demo.

The error log from Chrome on the error being reported:

Uncaught TypeError: Cannot read properties of null (reading 'style') 

    at fit (Playground.js:823:1) 

    at onload (VM8 smachines-hosted-activity.json&privaterepo=true&code=2b6213509b9dcb0922b0&state=4h5a3lzj7fg:1:1) 

ProgramPanel.js:17 evl 

ProgramPanel.js:17 flexmi 

ProgramPanel.js:17 emfatic 

metro.js:7426 Metro 4 - v4.5.1. Built at: 23/06/2022 09:21:11 

metro.js:7427 m4q - v1.1.0. Built at 05/05/2021 22:47:56 

/mif/metro.woff:1  

       Failed to load resource: the server responded with a status of 404 () 

/mif/metro.ttf:1  

       Failed to load resource: the server responded with a status of 404 () 

Playground.js:979 The contents of panel 'panel-evl' were saved successfully. 

Playground.js:979 The contents of panel 'panel-mm' were saved successfully. 

Playground.js:979 The contents of panel 'panel-model' were saved successfully. 

Playground.js:979 The contents of panel 'panel-evl' were saved successfully. 

Playground.js:979 The contents of panel 'panel-mm' were saved successfully. 

/mdenet-auth/github/file:1  

       Failed to load resource: the server responded with a status of 500 ()        <----- **Likely to be something to do with this** 

Playground.js:906 ERROR: An error occurred while trying to save the panel contents. 

/?activities=https:/…privaterepo=true#:1 Uncaught (in promise) Object.
szschaler commented 8 months ago

I suspect that the problem here might be that when creating the commit on GitHub, we need to provide the parent SHA. We currently do this by storing that SHA in the panel and providing it at the time of saving, but we do not update this SHA when a panel has been saved. As a result, the second save attempts to create a commit in a branch but that commit doesn't have the current branch HEAD as its parent. I suspect GitHub doesn't like that :-)

In fact, storing the SHA in the panel is wrong, it should really be a property of the platform -- or even be retrieved afresh as we create the commit.

szschaler commented 8 months ago

Looking at https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28 I think it's clear that my suspicion above is valid. The correct approach would be not to save the SHA in the panel at all but to instead pick up the current SHA in FileManager#storeFile before actually creating a commit. This needs to manage the situation where the file doesn't actually exist in the repository yet. Once we implement support for #140, we will simply use the HEAD SHA for the branch that we're saving to.

agarciadom commented 8 months ago

One thing to watch out for: what if I am a little forgetful, and I have the same activity open from multiple tabs? If the client is completely unaware of SHA1s, this could happen:

  1. Open activity on tab TA.
  2. Open activity on tab TB.
  3. Make changes on tab TA.
  4. Make changes on tab TB.
  5. Save from tab TA.
  6. Save from tab TB, which overwrites what I did on tab TA without a warning.

It may be worth for the tab to submit a request saying "I want to add this content on top of commit X", and the backend saying "yes, commit X is the latest, I have saved, here is the new SHA1", or "no, commit X is not the latest anymore, user should be shown a warning and be asked to either reload or confirm they want to overwrite their changes".