diff --git a/changelogs/unreleased/update-page-check-page.yml b/changelogs/unreleased/update-page-check-page.yml new file mode 100644 index 0000000000000000000000000000000000000000..ca9784a359b22906caea61875af6e2d838c1ab24 --- /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 85e8879bbc166ab4e220a1f5f2a4a80f3982f98b..4df9c81aac9f931378b44265e5d2895652aa4b20 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 696b7d84e885226ea3c6e7dfad12ef6abc1dd9f1..903c719a8cec0d5ad16fc05ead5ce578bac37edd 100644 --- a/internal/service/wiki/update_page_test.go +++ b/internal/service/wiki/update_page_test.go @@ -146,6 +146,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{ diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index bb31492926378f8e666b07a31b23532b5f03125e..69dddb01c60122036512233793a946f81d4bd48d 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 08292df97d54e6db0aa8542a84c392d3a630f101..9069968adb2c8ac1a9bd5205d533eb2eb1951748 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