Skip to content
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

Fix/merge wiki content into page #12333

Merged
merged 8 commits into from
May 11, 2023
Merged

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented Mar 25, 2023

Merges the WikiContent model into the WikiPage model. Presumably, the two were separate to support multiple text version of the same page but that reason was no longer relevant with the introduction of the journals. Nowadays, having the two separated does not grant any benefits while increasing complexity.

https://community.openproject.org/projects/openproject/work_packages/32796

The PR does not attempt to alter the behaviour of application in any way.

For the most part, the changes are just the removals of indirections (referencing the wiki page instead of the wiki page's content). In the frontend however a specific view object (WikiPages::AtVersion) has been introduced. Some views, most notably the show pages allow for browsing through different versions of the text. Instead of having multiple objects referenced in the view, the WikiPages::AtVersion object is passed, representing a wiki page at the version requested by the user. Since this affects the whole page, using ViewComponents has not seemed like an appropriate fit. There is no other view component as of now yet which is why the class is placed under app/helpers.

Limitations existing before have been kept. E.g. the journals only cover changes in the text and not in the title or the parent page. The author field existing both in the page (used to be content) as well as in the journal has also been kept. They arguable are unnecessary since the user reference is kept in the journal anyway.

TODO

  • Move code from wiki_content.rb into wiki_page.rb
  • Write migration to
    • alter the table structure of both wiki_page as well as rename wiki_content_journals into wiki_page_journals. Does not lead to more attributes being journalized but allows to later include e.g. the title or the parent page in the journalization.
    • migrate the existing data
  • Adapt the services
  • Adapt the contracts
  • Adapt the controller
  • Adapt the views  

@ulferts ulferts force-pushed the fix/merge_wiki_content_into_page branch 3 times, most recently from a8b6a17 to 95cb662 Compare April 3, 2023 19:20
@ulferts ulferts force-pushed the fix/merge_wiki_content_into_page branch from 95cb662 to 56dcb1e Compare April 4, 2023 15:58
@ulferts ulferts marked this pull request as ready for review April 5, 2023 07:14
@ulferts ulferts force-pushed the fix/merge_wiki_content_into_page branch from 56dcb1e to a328940 Compare April 5, 2023 08:05
@dombesz dombesz self-requested a review April 24, 2023 11:23
Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice refactoring on the wiki pages, congrats @ulferts !🎉
I left a few findings with screenshots.

app/views/wiki/show.html.erb Show resolved Hide resolved
app/controllers/wiki_controller.rb Show resolved Hide resolved
@ulferts ulferts force-pushed the fix/merge_wiki_content_into_page branch 2 times, most recently from dcecda8 to 67d3c98 Compare May 2, 2023 14:42
This is done, so former versions can be rolled back to. The transformation of the view object to it`s wrapped model needs to be prevented.

Once that had been achieved more indirections via content were removable.
@ulferts ulferts force-pushed the fix/merge_wiki_content_into_page branch from 67d3c98 to b22be95 Compare May 3, 2023 07:55
@dombesz dombesz self-requested a review May 9, 2023 10:40
Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉
Feel free to merge once CI is green.

@ulferts ulferts merged commit d369d3e into dev May 11, 2023
11 checks passed
@ulferts ulferts deleted the fix/merge_wiki_content_into_page branch May 11, 2023 08:53
@estan
Copy link

estan commented Aug 21, 2023

@ulferts In attempting to upgrade our instance to 13, we're getting

seeder_1    | PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_journals_on_journable_type_and_journable_id_and_version"
seeder_1    | DETAIL:  Key (journable_type, journable_id, version)=(WikiPage, 1, 1) already exists.

during the db/migrate/20230322135932_merge_wiki_content_into_page.rb migration added in this PR.

(When trying to google this problem I came across an old PR that may or may not be relevant: #9598)

@estan
Copy link

estan commented Aug 21, 2023

@estan
Copy link

estan commented Aug 21, 2023

Apologies, I see this is likely fixed with 1a640c2 already.

EDIT: Actually no, that commit is in v13.0.0, so that's not a fix, or the fix is not working somehow.

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