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

IBX-8418: Remove draft when trashing or deleting its parent or ancestor location #439

Merged
merged 3 commits into from
Nov 8, 2024

Conversation

barw4
Copy link
Contributor

@barw4 barw4 commented Oct 17, 2024

🎫 Issue IBX-8418

Continuation of #398

Related PRs:

ibexa/admin-ui#1321
https://github.com/ibexa/content-tree/pull/85

Description:

Currently, drafts without a location (so not yet published) that are orphaned due to missing ancestor location are forever stuck in the void. They cause multiple issues in different parts of DXP and they are not easily removable.

This PR makes sure that every time a location is trashed drafts under this given location or its child locations are removed.

Documentation:

Probably required

image

@barw4 barw4 self-assigned this Oct 17, 2024
@barw4 barw4 added Bug Something isn't working Ready for review labels Oct 17, 2024
@barw4
Copy link
Contributor Author

barw4 commented Oct 17, 2024

@katarzynazawada question:

  1. Question: @barw4, @NataliaBecla This notification about removing draft is displayed for every content, even if it does not have any drafts. By comparison, notification of the deletion of sub-items is only shown for content that has sub-items. It is expected behaviour?
  2. When removing content from the content tree there is no checkbox with I understand the consequences of this action. label.
Screenshot 2024-10-07 at 11 57 20
  1. When removing content from sub-items list there is no notification about drafts. Draft are removed but it could be confusing for the user.
Screenshot 2024-10-07 at 11 24 48
  1. For User content Item there is an inconsistency. When removing user using the button in menu there is modal without draft notification, when removing user using content tree this notification is displayed. (I am not sure if users have drafts at all.)
Screenshot 2024-10-07 at 11 57 20 Screenshot 2024-10-07 at 11 57 42
  1. When deleting content using API and DELETE /content/objects/{contentId} endpoints drafts are not removed, which can still cause the problems described in the ticket.

@barw4 barw4 requested a review from a team October 17, 2024 17:00
@barw4
Copy link
Contributor Author

barw4 commented Oct 17, 2024

@katarzynazawada

Question: @barw4, @NataliaBecla This notification about removing draft is displayed for every content, even if it does not have any drafts. By comparison, notification of the deletion of sub-items is only shown for content that has sub-items. It is expected behaviour?

Yes, this is expected. We have no information about number of drafts available under each tree and preparing a mechanism for counting that would require a lot of work.

@barw4 I do agree with @katarzynazawada here. Maybe something to address in the future in separate task.

When removing content from the content tree there is no checkbox with I understand the consequences of this action. label.

This is rather a design question but that would mean the checkbox would have to be checked every time we are trashing something, that would be cumbersome.
@barw4 We should be consistent here. Action like "Send to trash" should have same dialog/text and checkbox(if needed) to keep clarity and consistency for users.

When removing content from sub-items list there is no notification about drafts. Draft are removed but it could be confusing for the user.

We also don't have information about child contents or these messages that we have when trashing from content tree. These messages probably require refactor but I'm not convinced this should be the scope of this PR.

For User content Item there is an inconsistency. When removing user using the button in menu there is modal without draft notification, when removing user using content tree this notification is displayed. (I am not sure if users have drafts at all.)

Same as above. When user has child contents we are not showing any warnings like we do when trashing standard contents.

When deleting content using API and DELETE /content/objects/{contentId} endpoints drafts are not removed, which can still cause the problems described in the ticket.

Fixed

@katarzynazawada
Copy link

When removing content from the content tree there is no checkbox with I understand the consequences of this action. label.

This is rather a design question but that would mean the checkbox would have to be checked every time we are trashing something, that would be cumbersome.

From my point of view, the messages should be the same. If we perform the same action (deleting content) from different places in the system (a button in the menu or the content tree) the flow should be the same. Forcing an additional action on the user (ticking a checkbox) in one place and not in another can be confusing. @NataliaBecla please add your recommendation

When removing content from sub-items list there is no notification about drafts. Draft are removed but it could be confusing for the user.

For User content Item there is an inconsistency. When removing user using the button in menu there is modal without draft notification, when removing user using content tree this notification is displayed. (I am not sure if users have drafts at all.)

Indeed, there is also no information about removing subitems. I will report it in a separate ticket.

@NataliaBecla
Copy link

Question: @barw4, @NataliaBecla This notification about removing draft is displayed for every content, even if it does not have any drafts. By comparison, notification of the deletion of sub-items is only shown for content that has sub-items. It is expected behaviour?

Yes, this is expected. We have no information about number of drafts available under each tree and preparing a mechanism for counting that would require a lot of work."

@barw4 I do agree with @katarzynazawada here. Maybe something to address in the future in separate task.

When removing content from the content tree there is no checkbox with I understand the consequences of this action. label.

This is rather a design question but that would mean the checkbox would have to be checked every time we are trashing something, that would be cumbersome.

@barw4 We should be consistent here. Action like "Send to trash" should have same dialog/text and checkbox(if needed) to keep clarity and consistency for users.

Copy link

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

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

  1. Need to be fixed.
  2. Confirmed, it works now.

@NataliaBecla
Copy link

NataliaBecla commented Oct 23, 2024

@barw4 Can you please adjust the spacing between the title/question and the rest of the content, and change the font style slightly to make it more distinct. These small changes will significantly improve the presentation.
Screenshot 2024-10-23 at 15 34 01

@barw4
Copy link
Contributor Author

barw4 commented Oct 25, 2024

@katarzynazawada @NataliaBecla fixed and UI was improved. It's ready for re-test

@barw4 barw4 force-pushed the ibx-8418-delete-orphaned-drafts branch from ef4e9c6 to e55cc6b Compare November 8, 2024 13:59
Copy link

sonarqubecloud bot commented Nov 8, 2024

@alongosz alongosz merged commit 02200e6 into 4.6 Nov 8, 2024
21 checks passed
@alongosz alongosz deleted the ibx-8418-delete-orphaned-drafts branch November 8, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants