Handle error on merge conflict #383
Conversation
for more information, see https://pre-commit.ci
|
Hey @balajialg and @ryanlovett, this still needs a bit more work, but the proof of concept is there. I would love to get a bit of feedback from you as to whether you think a student would find this useful or not – I am trying to be careful with the wording here and not assume any prior git knowledge. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@yuvipanda I've gotten positive feedback via email from @balajialg and incorporated their suggestions, so this one is ready for your approval. |
nbgitpuller/static/js/giterror.js
Outdated
| url.searchParams.append("backup", "true"); | ||
|
|
||
| if (s.includes("merge")) | ||
| return `<p class="lead">Unresolvable conflicts detected while syncing</p><p><strong>Proceed without syncing</strong> to continue with the current state of your repository without updates.</p><p><strong>(Recommended) Backup and resync</strong> to backup the current state of your repository and sync updates into a new separate folder:<ul> |
There was a problem hiding this comment.
A quick response here is that this looks prone to cross site scripting attacks since the url and other things are end user controlled. I'll take a more detailed look later, but can you check that to see?
There was a problem hiding this comment.
The URL parameter backup only triggers code I wrote that is executed server-side, so I think I am okay with that being safe.
The error message contained in s comes from an event source listening to the backend running the git operation (and is the same one overlaid on top of the progress bar) so I don't think an attacker can hijack this to return an error message of their choosing.
The helper message
return `<p class="lead">Unresolvable conflicts detected while syncing</p...`is not user controlled. It is rendered via innerHTML, but since the content is statically authored by me, I don't think this introduces an XSS vuln. However, if this content were ever made dynamic in the future, it would need to be sanitised.
It's possible that I am missing something though – let me know if so.
|
Up to the caveat of internationalization for our French speaking students here, this looks very clear and simple, and a nice improvement on top of @cmarmo's previous step. Given the base line in the current release (where the students really don't know what to do in case of failure), and if there is no technical security barrier, I'd would be up for having this merged with a new release ASAP. Then, I'll be happy to test it on the battlefield with our hundreds of students and provide feedback to polish it more if needed :-) |
|
Looking at the screen again: I would put first the explanation (merge conflict), and then the two buttons giving the choice of action for the user (proceed without syncing / Backup and resync), both in orange/warning style (as both are not dangerous but imperfect). I am not sure about the "copy error" button. It's not a dangerous action (so not in red). It's not an alternative to the two other actions either. It's natural to keep it close to the error message. Maybe just having a "copy" icon to the right of the error message, close to the |
|
Hey @nthiery thank you for your feedback 🙏 I agree with your comments, I will iterate on the UI and come back with more consistent button placement and stylings. |
|
I've updated the top card with screenshots of the UI changes :) |
|
That was quick! Presumably the two buttons can be on the same line to save vertical space. And highlight that it's about choosing between the two. I am not sure whether one of the two options should be more recommended than the other. In my use cases, I would tend to recommend to not sync: not receiving the latest improvements to the training material is a slight annoyance. But having to juggle with one's work that has been moved away can be pretty confusing if the student does not understand what happened; and some (fragile) work to put things back in place. I can hear the 'I lost all my work!'. |
| """ | ||
| with Remote() as remote, Pusher(remote) as pusher: | ||
| pusher.push_file('README.md', '1') | ||
| puller = Puller(remote) |
There was a problem hiding this comment.
I think the Puller class is intended to be invoked as a context manager.
There was a problem hiding this comment.
I need two different Puller objects since
- the first one does the initial sync with
backupnot set - the second one triggers the merge conflict with
backup=true.
If nested two different context managers to invoke them, then the exiting the first one would fail because the backup strategy would rename the first puller.path and so there would be no temporary folder to cleanup in the puller.__exit__ teardown.
tests/test_gitpuller.py
Outdated
| puller_backup.__exit__ | ||
| puller.__exit__ |
There was a problem hiding this comment.
If both puller and puller_backup are invoked as context managers, these won't be needed! Although, right now they're not actually being invoked here.
There was a problem hiding this comment.
Hmm yeah you're right, puller.__exit__ would at least complain if I thought it was trying to rmtree a folder that was renamed and no longer exists. I'll double-check that cleanup is happening as I expect.
There was a problem hiding this comment.
K yeah, I've opted to just cleanup the pulled folders with af44677 and get rid of the unused .__exit__ calls.
Thank you for this perspective. I had high-pressure exam situations in mind (@balajialg) when making this recommendation, but with your feedback I will leave these 2 options as neutral since context matters here when making this choice. |
4 participants
todo
Closes #280 and closes 2i2c-org/infrastructure#7085