Empty wiki pages should be valid
Problem to solve
Creating a new Wiki page from the UI requires the content of the page to exist while creating the page via the command line does not.
Proposal
Make the Wiki page content field optional when creating a new page.
We should still prevent the value from being nil
but an empty string ""
should be acceptable.
Documentation
Testing
https://sentry.gitlab.net/gitlab/gitlabcom/issues/1637330/?referrer=gitlab_plugin
WikiPage::Meta::WikiPageInvalid: WikiPage::Meta::WikiPageInvalid
(105 additional frame(s) were not displayed)
...
wiki_pages/event_create_service.rb:16:in `block in execute'
wiki_page_meta = WikiPage::Meta.find_or_create(slug, page)
gitlab/metrics/instrumentation.rb:161:in `find_or_create'
.measure { super }
gitlab/metrics/method_call.rb:36:in `measure'
retval = yield
gitlab/metrics/instrumentation.rb:161:in `block in find_or_create'
.measure { super }
wiki_page/meta.rb:38:in `find_or_create'
raise WikiPageInvalid unless wiki_page.valid?
WikiPage::Meta::WikiPageInvalid
Designs
- Show closed items
Relates to
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- Markus Koller added Category:Wiki devopscreate + 1 deleted label
added Category:Wiki devopscreate + 1 deleted label
- Markus Koller added workflowplanning breakdown label
added workflowplanning breakdown label
- Markus Koller added backend label
added backend label
- Contributor
This error is raised in https://gitlab.com/gitlab-org/gitlab/blob/727282423530563a78f160e788e60e3edf84252a/app/models/wiki_page/meta.rb#L38 when the
WikiPage
has validation errors.Luckily the project in https://sentry.gitlab.net/gitlab/gitlabcom/issues/1637330/events/31209621/ is public, so I was able to clone the wiki and look at the commit hashes passed to the
PostReceive
worker. Turns out the commit adds an empty file, which fails our presence validation onWikiPage#content
.We could either make
content
optional in all cases, or tweak the validation so it only applies for edits made through GitLab (web UI, and probably API too)./cc @alexkalderimis
Collapse replies - Contributor
Thanks @toupeira - that is fascinating. We should of course allow the empty string as wiki content!
The correct approach would be to change the validation to prohibit
nil
, but allow the empty string, and add tests for this.This cannot be encountered using the Web UI, since we get this helpful (and scary looking) error instead:
- Contributor
@alexkalderimis browsers still send an empty string for blank fields though, no?
@fjsanpedro I'm curious if you ran into this with snippets as well, I see we still have a presence validation on
Snippet
but is that skipped in Git pushes, or are we blocking empty files completely there? - Contributor
@toupeira at the moment we have the presence validation in Snippet but we're moving away from it because, as you pointed out, you can create empty snippet files from git.
The plan is to enforce it through the FE but, in the BE, we're going to allow empty files.
- Contributor
@fjsanpedro thanks, that sounds reasonable!
So I think we can do the same for wikis:
- Require content to be present when editing in the UI.
- Allow empty files when using the API or pushing via Git.
/cc FYI @cdybenko @mnearents
- Contributor
browsers still send an empty string for blank fields
Yes indeed - I meant that the validation is being applied before we save, so we do not encounter this when trying to create the metadata.
Thanks @fjsanpedro and @toupeira - I think we have a good path forwards here.
- Contributor
@cdybenko @toupeira Ideally a user could create an empty wiki page. Because there's no harm in creating an empty page, we shouldn't make it an error (they're not losing data or doing anything destructive). They might be creating an empty placeholder page for someone else to edit, or to link to from another page.
- Contributor
@mnearents I agree that creating empty pages is a structure thing and makes sense to title and leave blank.
2 - Contributor
title of issue updated accordingly
- 🤖 GitLab Bot 🤖 mentioned in issue #222217 (closed)
mentioned in issue #222217 (closed)
- Alex Kalderimis changed title from WikiPage::Meta::WikiPageInvalid: WikiPage::Meta::WikiPageInvalid to Empty wiki pages should be valid
changed title from WikiPage::Meta::WikiPageInvalid: WikiPage::Meta::WikiPageInvalid to Empty wiki pages should be valid
- 🤖 GitLab Bot 🤖 mentioned in issue #223637 (closed)
mentioned in issue #223637 (closed)
- 🤖 GitLab Bot 🤖 mentioned in issue #225117 (closed)
mentioned in issue #225117 (closed)
- Luke Duncalfe added priority3 severity2 labels
- 🤖 GitLab Bot 🤖 mentioned in issue #226944 (closed)
mentioned in issue #226944 (closed)
- Alex Kalderimis changed milestone to %Backlog
changed milestone to %Backlog
- 🤖 GitLab Bot 🤖 added [deprecated] Accepting merge requests label
added [deprecated] Accepting merge requests label
- Darva Satcher set weight to 1
set weight to 1
- Darva Satcher added workflowscheduling label and removed workflowplanning breakdown label
added workflowscheduling label and removed workflowplanning breakdown label
- Darva Satcher set weight to 2
set weight to 2
- Darva Satcher added documentation label
added documentation label
- John Skarbek marked this issue as related to gitlab-com/gl-infra/production#2360 (moved)
marked this issue as related to gitlab-com/gl-infra/production#2360 (moved)
- 🤖 GitLab Bot 🤖 added sectiondev label
added sectiondev label
- Markus Koller marked #248565 (closed) as a duplicate of this issue
marked #248565 (closed) as a duplicate of this issue
- Markus Koller marked this issue as related to #248565 (closed)
marked this issue as related to #248565 (closed)
- Markus Koller mentioned in issue #248565 (closed)
mentioned in issue #248565 (closed)
- 🤖 GitLab Bot 🤖 added SLOMissed label
added SLOMissed label
- Eric Schurter added groupeditor [DEPRECATED] label and removed 1 deleted label
added groupeditor [DEPRECATED] label and removed 1 deleted label
- Francisco Javier López added Backlog RefinementEditor label
added Backlog RefinementEditor label
- Contributor
Backlog Refinement Session Update
DRI for next Steps: @ericschurter
Notes:
- Use case is for structuring the Wiki before editing content
- Workaround is easy, but annoying to have to remove placeholder
- Need a decision on whether we should make this available in the UI. There doesn't seem like much harm in doing so.
Next Steps:
- Update Description/Title
- Needs PM/UX Decision
- Technical Discovery
- Add to Strategy Epic
- Add to Tech Debt Epic
- Recommend Close
- Eric Schurter assigned to @ericschurter
assigned to @ericschurter
- Eric Schurter removed Backlog RefinementEditor label
removed Backlog RefinementEditor label
- Contributor
@mle Unless you have a different take, I think we should allow Wiki pages to be created from the UI with no body content. The use case here is around creating stubbed-out pages and establishing hierarchy for a wiki. It seems that it's possible via the command line, so there is no real reason we should limit this from the UI.
I believe making the content field optional would be sufficient.
Collapse replies - Developer
@ericschurter Yes creating empty pages should be considered as a valid page.
- Contributor
Great, thanks @mle
I've updated the description to be more specific about allowing this.
It's not critical but I'll keep it in the UX improvement epic for Wiki and we can pick it up sometime soon.
1
- Eric Schurter changed the description
Compare with previous version changed the description
- Eric Schurter added to epic &5341
added to epic &5341
- Eric Schurter unassigned @ericschurter
unassigned @ericschurter
- Contributor
@fjsanpedro let's try and get this in %14.2
I don't believe there's any UI work to be done, we just need to make enable
Save
button even when the description field is empty. Collapse replies - Contributor
Yes, it's only backend work and that milestone is totally fine.
- Eric Schurter changed milestone to %14.2
changed milestone to %14.2
- Eric Schurter mentioned in issue gitlab-com/www-gitlab-com#11349 (closed)
mentioned in issue gitlab-com/www-gitlab-com#11349 (closed)
- Francisco Javier López added frontend label
added frontend label
- Francisco Javier López mentioned in merge request !65521 (merged)
mentioned in merge request !65521 (merged)
- Francisco Javier López assigned to @fjsanpedro
assigned to @fjsanpedro
- Francisco Javier López changed milestone to %14.1
changed milestone to %14.1
- Francisco Javier López added workflowin dev label and removed workflowscheduling label
added workflowin dev label and removed workflowscheduling label
- David O'Regan mentioned in issue #335031 (closed)
mentioned in issue #335031 (closed)
- 🤖 GitLab Bot 🤖 removed [deprecated] Accepting merge requests label
removed [deprecated] Accepting merge requests label
- Francisco Javier López added workflowin review label and removed workflowin dev label
added workflowin review label and removed workflowin dev label
- Francisco Javier López added workflowverification label and removed workflowin review label
added workflowverification label and removed workflowin review label
- Francisco Javier López removed SLOMissed label
removed SLOMissed label
- Contributor
@oregand the backend MR (!65521 (merged)) has been merged. We need some frontend work to allow empty pages in the Content Editor and also in the old editor.
I'm removing the backend label from this issue.
Edited by Francisco Javier López 1 Collapse replies - Maintainer
Thank you @fjsanpedro
For a severity2 what kind of a schedule should this be considered with? Do we look to see if we have the capacity to handle it now based on the LOE or schedule it in for %14.3 perhaps?
- Contributor
@oregand It would be great to fit this now so it doesn't linger but I know it's tight at the moment. If no one can finish it up now, I think 14.3 is fine.
- Maintainer
Do either of y'all have the capacity to look at the frontend side of this? :)
- Maintainer
@oregand I have a few issues/MRs in progress or needed to start ahead of this. Unless you want me to prioritize it, I probably would not be able to look at it until next week.
- Maintainer
Thank you for the update, next week would be fine
Edited by David O'Regan - Maintainer
FYI @ericschurter - after discussion with @oregand , I'm deprioritizing this in favor of focusing on wrapping up the remaining CAPTCHA refactors and documentation.
This means there's a chance it might not make it into 14.2.
- Maintainer
We're going to push this to %14.3
- Contributor
Just for the sake of completeness, @oregand and I discussed this and the other issue from 14.2 that are at risk. I'm 100% behind pushing this to 14.3 in order to focus on your other work.
- Maintainer
@oregand @ericschurter given recent reallocations, should we push this out to a further release (and unassign me and Fran for now until it comes back up in an upcoming release)?
- Contributor
Indeed @cwoolley-gitlab, it will get pushed out. @oregand and I will update all the deliverables in the next day or two, most likely assigning them to %14.6.
1 - Maintainer
unassigning myself until this gets rescheduled.
- Francisco Javier López removed backend label
removed backend label
- 🤖 GitLab Bot 🤖 added SLOMissed label
added SLOMissed label
- David O'Regan assigned to @cwoolley-gitlab
assigned to @cwoolley-gitlab
- Francisco Javier López mentioned in issue create-stage#12845 (closed)
mentioned in issue create-stage#12845 (closed)
- Francisco Javier López added workflowin dev label and removed workflowverification label
added workflowin dev label and removed workflowverification label
- Francisco Javier López added workflowready for development label and removed workflowin dev label
added workflowready for development label and removed workflowin dev label
- John Hope mentioned in issue create-stage#12887 (closed)
mentioned in issue create-stage#12887 (closed)
- Francisco Javier López changed milestone to %14.2
changed milestone to %14.2
- David O'Regan changed milestone to %14.3
changed milestone to %14.3
- David O'Regan added Deliverable label
added Deliverable label
- Eric Schurter changed milestone to %14.6
changed milestone to %14.6
- Eric Schurter removed Deliverable label
removed Deliverable label
- Chad Woolley unassigned @cwoolley-gitlab
unassigned @cwoolley-gitlab
- Francisco Javier López changed milestone to %14.8
changed milestone to %14.8
- Maintainer
Collapse replies - Contributor
Unassigning Fran and maybe we can get to this in %14.10
- Contributor
@ericschurter can this be scheduled for upcoming release? This came up as one of the oldest severity2 issues in Editor
- Contributor
@at.ramya We don't have capacity to work on this at the moment, but thanks for highlighting it. I'll update the milestone to prevent confusion.
I'd also question the severity on this, as it's a minor inconvenience to include a space or other character in the field before submitting.
- Contributor
Thanks, Eric.
Reducing the severity
- David O'Regan changed milestone to %14.9
changed milestone to %14.9
- David O'Regan added Stretch label
added Stretch label
- Eric Schurter changed milestone to %14.10
changed milestone to %14.10
- Eric Schurter unassigned @fjsanpedro
unassigned @fjsanpedro
- 🤖 GitLab Bot 🤖 added [deprecated] Accepting merge requests label
added [deprecated] Accepting merge requests label
- Tanya Pazitny added 1 deleted label
added 1 deleted label
- 🤖 GitLab Bot 🤖 changed milestone to %15.0
changed milestone to %15.0
- 🤖 GitLab Bot 🤖 added missed:14.10 label
added missed:14.10 label
- 🤖 GitLab Bot 🤖 changed milestone to %15.1
changed milestone to %15.1
- 🤖 GitLab Bot 🤖 added missed:15.0 label
added missed:15.0 label
- Eric Schurter changed milestone to %Backlog
changed milestone to %Backlog
- Ramya Authappan added severity3 label and removed severity2 label
- 🤖 GitLab Bot 🤖 added groupknowledge label and removed groupeditor [DEPRECATED] label
added groupknowledge label and removed groupeditor [DEPRECATED] label
- 🤖 GitLab Bot 🤖 added devopsplan label and removed devopscreate label
added devopsplan label and removed devopscreate label
- 🤖 GitLab Bot 🤖 added vintage label
added vintage label
- Maintainer
Hi @jobbot
,Thanks for raising this bug.
Contributions like this are vital to help make GitLab a better product.
We would be grateful for your help in verifying whether your bug report requires further attention from the team. If you think this bug still exists, and is reproducible with the latest stable version of GitLab, please comment on this issue.
This issue has been inactive for more than 12 months now and based on the policy for inactive bugs, will be closed in 7 days.
Thanks for your contributions to make GitLab better!
- 🤖 GitLab Bot 🤖 added stale label
added stale label
- Maintainer
Hi @jobbot
,Based on the policy for inactive bugs, this issue is now being closed.
If you think this bug still exists, and is reproducible with the latest stable version of GitLab, please reopen this issue.
Thanks for your contributions to make GitLab better!
- 🤖 GitLab Bot 🤖 closed
closed
- 🤖 GitLab Bot 🤖 added auto closed label
added auto closed label
- Matthew Macfarlane reopened
reopened
- Matthew Macfarlane added typefeature label and removed typebug label
added typefeature label and removed typebug label
- Matthew Macfarlane removed priority3 label
removed priority3 label
- Matthew Macfarlane removed sectiondev label
removed sectiondev label
- 🤖 GitLab Bot 🤖 added sectiondev label
added sectiondev label
- Matthew Macfarlane removed severity3 label
removed severity3 label
- Matthew Macfarlane removed SLOMissed label
removed SLOMissed label
- Matthew Macfarlane removed [deprecated] Accepting merge requests label
removed [deprecated] Accepting merge requests label
- Developer
Re-opening this Issue as we evaluate improvements to GitLab Wiki. Also adjusting this from a typebug to a typefeature.
- Matthew Macfarlane mentioned in issue #452225 (closed)
mentioned in issue #452225 (closed)
- Maintainer
This is now resolved. Thanks to @annabeldunstone for fixing this issue in !153331 (merged).
1 Collapse replies - Developer
Fantastic, Thank you @annabeldunstone
- Himanshu Kapoor closed
closed
- Himanshu Kapoor added workflowcomplete label and removed workflowready for development label
added workflowcomplete label and removed workflowready for development label
- Annabel Dunstone Gray assigned to @annabeldunstone
assigned to @annabeldunstone
- Annabel Dunstone Gray changed milestone to %17.1
changed milestone to %17.1
- Annabel Dunstone Gray added UX Paper Cuts label
added UX Paper Cuts label
- Himanshu Kapoor mentioned in issue #497419 (closed)
mentioned in issue #497419 (closed)