Allow the creation of Project Wikis using the internal provider#24195
Allow the creation of Project Wikis using the internal provider#24195mereghost wants to merge 17 commits into
Conversation
1269d5c to
464e5f6
Compare
464e5f6 to
f46bdd8
Compare
9da6890 to
8605b87
Compare
f516674 to
8a81432
Compare
|
Warning This pull request does not link an OpenProject work package. Please add a link to the work package in the description, or reference it in the |
|
Early feedback is welcome. It builds upon its parent branch. |
NobodysNightmare
left a comment
There was a problem hiding this comment.
Some first preliminary feedback
|
|
||
| def visible?(user = User.current) | ||
| !user.nil? && user.allowed_in_project?(:view_wiki_pages, project) | ||
| enabled && user&.allowed_in_project?(:view_wiki_pages, project) |
There was a problem hiding this comment.
Nothing that you introduced, but I have to admit that I find it very weird to have a parameter user that can be passed in, that has a default value that can never be nil and then still perform a nil check as the first thing.
There was a problem hiding this comment.
That was probably just some rubocop appeasing at some point.
Well... User.current could be nil
There was a problem hiding this comment.
I am pretty sure that it can't. We always fall back to the anonymous user...
There was a problem hiding this comment.
Even on test cases? 😮
There was a problem hiding this comment.
If not, the test cases would behave weird and we'd adapt code purely to work with test cases that behave different than reality.
I didn't check test cases, but I can confirm that opening a fresh rails console also gives me the anonymous user when calling User.current.
| return project.reload.wiki if Wiki.exists?(project: @project) | ||
|
|
||
| Wiki.create!(project: @project, start_page: "Wiki") | ||
| @project.reload |
There was a problem hiding this comment.
This method looks unfinished (aside from the TODO comment), I am wondering if it's doing what's intended:
- 3 places call it
@project, one place calls itproject - The guard clause returns the wiki after reloading the project, the last clause returns the reloaded project
- the return value is not used
Right now, I think what you need is
Wiki.create!(project: @project, start_page: "Wiki") unless Wiki.exists?(project: @project)
@project.reloadI also realize (and that's what your FIXME probably hints at) that this is the second place where a wiki is created during seeding, so maybe something can be improved/centralized here.
There was a problem hiding this comment.
Yeah, this whole process got messy right now. It was handled by AR hooks and now we need a proper implementation / solution.
| ::BasicData::PrioritySeeder, | ||
| ::Bim::BasicData::SettingSeeder, | ||
| ::Bim::BasicData::ThemeSeeder | ||
|
|
| Primer::Forms::ToggleSwitchForm.new( | ||
| name: "enable_interal_wiki", | ||
| label: t("projects.settings.internal_wiki.enable"), | ||
| caption: t("projects.settings.internal_wiki.caption"), |
There was a problem hiding this comment.
Probably a good case for component-relative translations?
I.e. t(".enable") and t(".caption")
| end | ||
|
|
||
| def wiki_enabled? | ||
| !!@project.wiki |
There was a problem hiding this comment.
That's a surprising consequence of a method overwrite I saw earlier.
I'd have expected that this would read @project.wiki.enabled?, because that would seem to make sense right?
However, the Project will act as if it has no wiki, unless the wiki is enabled. I guess that this is great for compatibility with existing code, but it definitely also leads to surprising places like this one.
There was a problem hiding this comment.
Agreed. The codebase used to check for the enabled module, now we need 2 different checks: Internal Provider and the Project Wiki.
I'll see if I can de-crapify some of the checks around it without breaking another 400 tests.
|
|
||
| create_or_update_wiki | ||
| replace_via_turbo_stream(component: ProjectInternalWikiComponent.new(@project.reload)) | ||
| render_success_flash_message_via_turbo_stream(message: "Internal wiki disenabled for this project.") |
| end | ||
|
|
||
| def create_or_update_wiki | ||
| if find_project_wiki |
There was a problem hiding this comment.
This is confusing. find_project_wiki is already a before_action, so this method was already executed and it would probably be enough to write if @wiki.
On the other hand, this is the only method that refers to @wiki at all, so I am wondering, whether this should be inlined or at least de-instance-variablified.
E.g.
def create_or_update_wiki
wiki = Wiki.find_by(project: @project)
if wiki
wiki.toggle!(:enabled)
else
Wiki.create!(project: @project, start_page: "Wiki")
end
endThere was a problem hiding this comment.
Oh... it is a remnant of a previous implementation. Good catch.
| settings: | ||
| internal_wiki: | ||
| caption: >- | ||
| This enables OpenProject's native wiki module, which can exist alongside external wiki providers. |
There was a problem hiding this comment.
Reminder to self that I wanted to talk to Parimal about this:
- mention of "module" makes this sentence unnecessarily technical
- should we even mention external wiki providers at all?
- the final sentence is complex and does not seem to be correct (e.g. we even hide existing wiki pages, if an external provider exists)
8853579 to
0b6493d
Compare
Co-authored-by: Jan Sandbrink <j.sandbrink@openproject.com>
649a32a to
28baa76
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
This covers parts of OP#XWI-52, OP#XWI-53 and OP#XWI-54 on the OP#XWI-42 feature.
What is missing at this moment: