Merge branch 'update-page-check-page' into 'master'

More informative error state when pages are not found

See merge request gitlab-org/gitaly!1340

5 files changed, 52 insertions, 15 deletions

diff --git a/changelogs/unreleased/update-page-check-page.yml b/changelogs/unreleased/update-page-check-page.yml
new file mode 100644
index 000000000..ca9784a35
--- /dev/null
+++ b/changelogs/unreleased/update-page-check-page.yml
@@ -0,0 +1,4 @@
+---
+title: More informative error states for missing pages
+merge_request: 1340
+type: fixed
diff --git a/internal/service/wiki/delete_page_test.go b/internal/service/wiki/delete_page_test.go
index 85e8879bb..4df9c81aa 100644
--- a/internal/service/wiki/delete_page_test.go
+++ b/internal/service/wiki/delete_page_test.go
@@ -111,7 +111,7 @@ func TestFailedWikiDeletePageDueToValidations(t *testing.T) {
 				PagePath:      []byte("does-not-exist"),
 				CommitDetails: commitDetails,
 			},
-			code: codes.Unknown,
+			code: codes.NotFound,
 		},
 		{
 			desc: "empty page path",
diff --git a/internal/service/wiki/update_page_test.go b/internal/service/wiki/update_page_test.go
index 696b7d84e..903c719a8 100644
--- a/internal/service/wiki/update_page_test.go
+++ b/internal/service/wiki/update_page_test.go
@@ -147,6 +147,18 @@ func TestFailedWikiUpdatePageDueToValidations(t *testing.T) {
 			code: codes.InvalidArgument,
 		},
 		{
+			desc: "page does not exist",
+			request: &gitalypb.WikiUpdatePageRequest{
+				Repository:    wikiRepo,
+				PagePath:      []byte("//Installing Gibaly"),
+				Title:         []byte("Installing Gitaly"),
+				Format:        "markdown",
+				CommitDetails: commitDetails,
+				Content:       []byte(""),
+			},
+			code: codes.NotFound,
+		},
+		{
 			desc: "empty title",
 			request: &gitalypb.WikiUpdatePageRequest{
 				Repository:    wikiRepo,
diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb
index bb3149292..69dddb01c 100644
--- a/ruby/lib/gitlab/git/wiki.rb
+++ b/ruby/lib/gitlab/git/wiki.rb
@@ -3,6 +3,7 @@ module Gitlab
     class Wiki
       DuplicatePageError = Class.new(StandardError)
       OperationError = Class.new(StandardError)
+      PageNotFound = Class.new(GRPC::NotFound)
 
       CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do
         def to_h
@@ -50,19 +51,6 @@ module Gitlab
         gollum_find_file(name, version)
       end
 
-      # options:
-      #  :page     - The Integer page number.
-      #  :per_page - The number of items per page.
-      #  :limit    - Total number of items to return.
-      def page_versions(page_path, options = {})
-        current_page = gollum_page_by_path(page_path)
-
-        commits_from_page(current_page, options).map do |gitlab_git_commit|
-          gollum_page = gollum_wiki.page(current_page.title, gitlab_git_commit.id)
-          Gitlab::Git::WikiPageVersion.new(gitlab_git_commit, gollum_page&.format)
-        end
-      end
-
       def count_page_versions(page_path)
         @repository.count_commits(ref: 'HEAD', path: page_path)
       end
@@ -131,11 +119,12 @@ module Gitlab
                         offset: options[:offset])
       end
 
+      # Retrieve the page at that `page_path`, raising an error if it does not exist
       def gollum_page_by_path(page_path)
         page_name = Gollum::Page.canonicalize_filename(page_path)
         page_dir = File.split(page_path).first
 
-        gollum_wiki.paged(page_name, page_dir)
+        gollum_wiki.paged(page_name, page_dir) || (raise PageNotFound, page_path)
       end
 
       def gollum_write_page(name, format, content, commit_details)
diff --git a/ruby/spec/lib/gitlab/git/wiki_spec.rb b/ruby/spec/lib/gitlab/git/wiki_spec.rb
index 08292df97..9069968ad 100644
--- a/ruby/spec/lib/gitlab/git/wiki_spec.rb
+++ b/ruby/spec/lib/gitlab/git/wiki_spec.rb
@@ -75,6 +75,35 @@ describe Gitlab::Git::Wiki do
     end
   end
 
+  describe '#update_page' do
+    let(:old_title) { 'page1' }
+    let(:new_content) { 'different content' }
+    let(:new_title) { 'new title' }
+    let(:deets) { commit_details('update') }
+
+    before do
+      create_page(old_title, 'some content')
+    end
+    after do
+      destroy_page(new_title)
+    rescue Gitlab::Git::Wiki::PageNotFound
+      destroy_page(old_title)
+    end
+
+    it 'can update the page' do
+      subject.update_page(old_title, new_title, :markdown, new_content, deets)
+
+      expect(subject.pages.count).to eq 1
+      expect(subject.pages.first.title).to eq new_title
+      expect(subject.pages.first.text_data).to eq new_content
+    end
+
+    it 'raises PageNotFound when trying to access an unknown page' do
+      expect { subject.update_page('bad path', new_title, :markdown, new_content, deets) }
+        .to raise_error(Gitlab::Git::Wiki::PageNotFound)
+    end
+  end
+
   def create_page(name, content)
     subject.write_page(name, :markdown, content, commit_details(name))
   end
@@ -85,6 +114,9 @@ describe Gitlab::Git::Wiki do
 
   def destroy_page(title, dir = '')
     page = subject.page(title: title, dir: dir)
+
+    raise Gitlab::Git::Wiki::PageNotFound, title unless page
+
     subject.delete_page(page.path, commit_details(title))
   end
 end