Skip to content

Commit

Permalink
Don't allow the creation of empty revisions with no changes
Browse files Browse the repository at this point in the history
  • Loading branch information
wlach committed Mar 6, 2019
1 parent f498a04 commit 475573a
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- set default document position to (row 1, column 1) on startup (#1568)
- displays owner name in header message when viewing a notebook you do not own
- randomly generate initial notebook names (based on a list of iodide compounds) (#1541)
- don't allow the creation of "empty" revisions on the server

# 0.2.0 (2019-02-28)

Expand Down
8 changes: 8 additions & 0 deletions server/notebooks/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ class NotebookRevisionDetailSerializer(serializers.ModelSerializer):
Details of a revision for a notebook (includes content)
"""

def validate(self, attrs):
last_revision = NotebookRevision.objects.filter(
notebook_id=self.context["notebook_id"]
).first()
if attrs["title"] == last_revision.title and attrs["content"] == last_revision.content:
raise serializers.ValidationError("Revision unchanged from previous")
return serializers.ModelSerializer.validate(self, attrs)

class Meta:
model = NotebookRevision
fields = ("id", "title", "created", "content")
Expand Down
38 changes: 38 additions & 0 deletions server/tests/test_notebook_revision_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,44 @@ def test_create_notebook_revision(fake_user, test_notebook, client):
test_notebook.refresh_from_db()
assert test_notebook.title == post_blob["title"]

# also validate that the response is what we expect
assert resp.json() == {
"content": post_blob["content"],
"created": get_rest_framework_time_string(new_notebook_revision.created),
"id": new_notebook_revision.id,
"title": post_blob["title"],
}


def test_dont_create_unmodified_notebook_revision(fake_user, test_notebook, client):
client.force_login(user=fake_user)

# get latest (only) revision for test notebook
notebook_revision = NotebookRevision.objects.first()
original_creation_time = notebook_revision.created
original_id = notebook_revision.id

# unmodified post
unmodified_post_blob = {"title": notebook_revision.title, "content": notebook_revision.content}

resp = client.post(
reverse("notebook-revisions-list", kwargs={"notebook_id": test_notebook.id}),
unmodified_post_blob,
)
assert resp.status_code == 400

# no new notebook revisions should be created, and the existing (only)
# revision should be unchanged
assert NotebookRevision.objects.count() == 1
notebook_revision = NotebookRevision.objects.first()
assert notebook_revision.title == unmodified_post_blob["title"]
assert notebook_revision.content == unmodified_post_blob["content"]
assert notebook_revision.created == original_creation_time
assert notebook_revision.id == original_id

# be sure that we get the expected error
assert resp.json() == {"non_field_errors": ["Revision unchanged from previous"]}


def test_delete_notebook_revision_not_logged_in(test_notebook, client):
# should not be able to delete a notebook revision if not logged in
Expand Down
41 changes: 28 additions & 13 deletions src/actions/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,22 +274,37 @@ function getNotebookSaveRequestOptions(state, options = undefined) {
}

function saveNotebookRequest(url, postRequestOptions, dispatch) {
return fetchWithCSRFTokenAndJSONContent(url, postRequestOptions)
.then(response => {
return fetchWithCSRFTokenAndJSONContent(url, postRequestOptions).then(
response => {
if (!response.ok) {
throw response;
return response
.json()
.then(json => {
// this is a horrible and obvious hack that will go away
// when we don't provide an easy interface for the user
// to double save a revision (i.e. we have provided
// continuous autosave)
if (json.non_field_errors && json.non_field_errors.length) {
// no changes, not really an error, we'll silently pretend
// this didn't happen
return {};
}
// else, some kind of genuine error
dispatch(
updateAppMessages({
message: "Error saving notebook.",
messageType: "ERROR_SAVING_NOTEBOOK"
})
);
throw response;
})
.catch(() => {
throw response;
});
}
return response.json();
})
.catch(err => {
dispatch(
updateAppMessages({
message: `Error Saving Notebook.`,
messageType: "ERROR_SAVING_NOTEBOOK"
})
);
throw new Error(err);
});
}
);
}

export function createNewNotebookOnServer(options = { forkedFrom: undefined }) {
Expand Down

0 comments on commit 475573a

Please sign in to comment.