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

#784 - Adds ability to remove particular revision. #1008

Merged
merged 7 commits into from Sep 16, 2018

Conversation

2 participants
@Abijeet
Member

Abijeet commented Sep 15, 2018

Hey @ssddanbrown ,

Been a little busy with my professional life and haven't had a chance to work on BookStack. Finally got some time this weekend.

Here's what's been done,

  1. Added a drop-down for deletion confirmation on the revision page.
  2. Wrote code to delete the revision
  3. Added test cases.

Note that I'm not reducing the revision_count column in the pages table. I think the current revision number should not be changed.

Screenshot

bookstack-revision-ddl-2

Abijeet added some commits Sep 15, 2018

Adds code to delete the revision.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>
Added a success message on deletion of revision.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>
Adds tests and few fixes.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>
Removes the BadRequestException class added earlier.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>

@Abijeet Abijeet added the Enhancement label Sep 15, 2018

@Abijeet Abijeet added this to the BookStack Beta v0.24.0 milestone Sep 15, 2018

@Abijeet Abijeet self-assigned this Sep 15, 2018

@Abijeet Abijeet requested a review from ssddanbrown Sep 15, 2018

@Abijeet Abijeet force-pushed the revision-deletion branch from 2f96962 to 0c8b6b7 Sep 15, 2018

@Abijeet

This comment has been minimized.

Member

Abijeet commented Sep 15, 2018

@ssddanbrown - Not entirely sure why the test cases are failing.

@ssddanbrown

Thanks for this @Abijeet. All looks great. Picked up a few things to review in the code and have found the reason for the failing test case.

Show resolved Hide resolved app/Http/Controllers/PageController.php
public function destroyRevision($bookSlug, $pageSlug, $revId)
{
$page = $this->entityRepo->getBySlug('page', $pageSlug, $bookSlug);
$this->checkOwnablePermission('page-update', $page);

This comment has been minimized.

@ssddanbrown

ssddanbrown Sep 16, 2018

Member

Think it may be better to check page-delete permission here instead since it's effectively a related delete operation?

// Check if its the latest revision, cannot delete latest revision.
if (intval($currentRevision->id) === intval($revId)) {
session()->flash('error', trans('entities.revision_cannot_delete_latest'));
return view('pages/revisions', ['page' => $page, 'book' => $page->book, 'current' => $page]);

This comment has been minimized.

@ssddanbrown

ssddanbrown Sep 16, 2018

Member

Since this is not a successful request it's probably best to set a non-successful status code here. Maybe a 403?

Can just add a status code as an extra parameter to the view function.

Making this change gives a clue as to why the test case fails.

return PageRevision::find($id);
}
return null;
}

This comment has been minimized.

@ssddanbrown

ssddanbrown Sep 16, 2018

Member

The revision table can contain also contain 'update drafts' (Autosaves) so querying the revisions directly like this may not return what you expect.

The $page->revisions relation (line 62) has the correct filtering applied and is ordered by created_at. Therefore you should be able to simplify this function the following:

return $this->revisions()->first()
Show resolved Hide resolved tests/Entity/PageRevisionTest.php
Changes as per code review, and fixes failing test cases.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>

@Abijeet Abijeet force-pushed the revision-deletion branch from bb3004f to 08b9676 Sep 16, 2018

@Abijeet Abijeet requested a review from ssddanbrown Sep 16, 2018

@Abijeet

This comment has been minimized.

Member

Abijeet commented Sep 16, 2018

Thanks for the review @ssddanbrown. Made the changes.

Returning status code 400 instead of 403 in case user tries to delete the current revision.

@Abijeet Abijeet merged commit 32e34f1 into master Sep 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Abijeet Abijeet deleted the revision-deletion branch Sep 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment