Skip to content

[#73354] add deletion confirmation dialog for page links#23193

Merged
Kharonus merged 14 commits into
devfrom
implementation/73354-add-deletion-action-to-wiki-page-link
May 21, 2026
Merged

[#73354] add deletion confirmation dialog for page links#23193
Kharonus merged 14 commits into
devfrom
implementation/73354-add-deletion-action-to-wiki-page-link

Conversation

@Kharonus
Copy link
Copy Markdown
Member

@Kharonus Kharonus commented May 13, 2026

Ticket

73354

What are you trying to accomplish?

  • added workflow of deleting relation page links
  • show confirmation dialog on click and call deletion action on submitting the dialog
  • ⚠️ Actual deletion not yet triggered, this PR is for introducing the deletion dialog and the corresponsing controllers, data and routes. Those are now up for discussion.

What approach did you choose and why?

  • add confirmation dialog component
  • add delete action and controller
  • add action to permission
  • add page link data to page info object
  • add additional routes

Hint

This PR is based on #23142, so the changes of the other still polutes the diff. Please review and merge the other first.

The major focus of this PR and the discussion involed is:

  • page info object now includes an optional page link reference. Reasoning here is, that page info is used as a view model for the page link representations in the UI - among other uses. Especially for the deletion data of the page link entity is needed (own id, wp and project reference for constructing the correct route).
    • one additional thought: should the page_link_component validate the input it gets? e.g. if there an action refined :remove, but the page info does not contain a page link entity, should we raise a meaningful message?
  • controller and route naming - are we fine with dedicated, focused controllers, i.e. separating the macro controller from the controller managing the relation page links of the wiki tab.
  • test coverage: this PR mostly contains UI and connection logic (permissions, actions, routes, etc.). Are any component/controller/unit tests needed here?

…to implementation/73354-add-deletion-action-to-wiki-page-link
@Kharonus Kharonus requested a review from a team May 13, 2026 13:17
@Kharonus Kharonus self-assigned this May 13, 2026
- https://community.openproject.org/work_packages/73354
- add confirmation dialog component
- add delete action and controller
- add action to permission
- add page link data to page info object
- add additional routes
- NOT ADDED: deletion service
@Kharonus Kharonus force-pushed the implementation/73354-add-deletion-action-to-wiki-page-link branch from 6ad6a9e to 75a9c95 Compare May 13, 2026 13:24
Comment thread modules/wikis/config/routes.rb Outdated
Comment thread modules/wikis/config/routes.rb Outdated
@Kharonus Kharonus marked this pull request as ready for review May 19, 2026 14:21
@Kharonus Kharonus requested a review from NobodysNightmare May 19, 2026 14:22
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 19, 2026

Caution

The provided work package version does not match the core version

Details:

Please make sure that:

  • The work package version OR your pull request target branch is correct

Copy link
Copy Markdown
Contributor

@NobodysNightmare NobodysNightmare left a comment

Choose a reason for hiding this comment

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

I left some review comments.

From a high level, I am not sure how much I like the idea of adding page_link to the page_info. I see that it was necessary to make link building easier (versus building a link from provider and identifier), but what I see critical is that we now have two classes of PageInfo objects. Those with a link and those without.

Adding to that is that not all places that could add a page link to the page info also do add it. For example the ReferencingPageLinks from the internal provider omit them. Do they do this, because we knew that not all future callers will be able to do so and this way we are just much more consistent in our results? Or did they do it, because they knew that the receivers would not need the page link?

Comment thread modules/wikis/app/components/wikis/page_link_component.rb
Comment thread modules/wikis/config/routes.rb Outdated
Comment thread modules/wikis/app/controllers/wikis/relation_page_links_controller.rb Outdated
Comment thread modules/wikis/app/views/wikis/admin/wiki_providers/edit.html.erb Outdated
Comment thread modules/wikis/config/locales/en.yml Outdated
Comment thread modules/wikis/config/routes.rb Outdated
@Kharonus Kharonus requested a review from NobodysNightmare May 20, 2026 16:21
Copy link
Copy Markdown
Contributor

@NobodysNightmare NobodysNightmare left a comment

Choose a reason for hiding this comment

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

LGTM. I did a final click test and this also looks good. Later on we can discuss whether we want to include some additional context in the dialog, because right now it's not as helpful as it could be and very much only whitespace:

Image

E.g. we could repeat the name of the page that's being linked to. But that's certainly something for "later".

@Kharonus Kharonus merged commit 76696fe into dev May 21, 2026
16 checks passed
@Kharonus Kharonus deleted the implementation/73354-add-deletion-action-to-wiki-page-link branch May 21, 2026 15:27
@github-actions github-actions Bot locked and limited conversation to collaborators May 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants