Skip to content

OF-1387 Add confirmation before deleting subscribers in pubsub admin UI#3215

Merged
guusdk merged 8 commits intoigniterealtime:mainfrom
MilanTyagi2004:OF-1387-fix-subscriber-delete-confirmation
Mar 25, 2026
Merged

OF-1387 Add confirmation before deleting subscribers in pubsub admin UI#3215
guusdk merged 8 commits intoigniterealtime:mainfrom
MilanTyagi2004:OF-1387-fix-subscriber-delete-confirmation

Conversation

@MilanTyagi2004
Copy link
Contributor

Fixes OF-1387.

This change adds a confirmation prompt before deleting subscribers in pubsub-node-subscribers.jsp to prevent accidental deletions.

The implementation uses a localized message (pubsub.node.subscribers.confirm_delete) and aligns with existing confirmation patterns used in the admin console.

This ensures a consistent and safer user experience across the UI.

@MilanTyagi2004
Copy link
Contributor Author

Hi @guusdk , I’ve implemented the fix for OF-1387. Let me know if any changes are needed.

Copy link
Member

@guusdk guusdk left a comment

Choose a reason for hiding this comment

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

Thanks for this change! Although technically, it seems to work, visually, something weird happens. Can you change this please?

Consider this node (with one affiliation), which is a node in a PEP service.

Image

When you click the red cross button, you get the a confirmation message, as expected. However, the affiliation has already disappeared and the information box also appears to have changed:

Image

This may be a result from this node being a node in a PEP service (which is a special kind of Pubsub service). I am not able to easily test this with a 'regular' Pubsub service, but regardless, the confirmation page should also work properly for PEP services.

My last bit of feedback: I don't quite like how the confirmation dialog shows "under" the data. I see that this is how it was mocked in the JIRA ticket, but it is not very consistent with other pages. They typically show a dedicated page for such a confirmation. Take, for example, this confirmation of deleting a roster item:

Image

I would prefer the new dialog to be more like this, to be more consistent with the rest of the admin console.

Copy link
Member

Choose a reason for hiding this comment

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

This is a trivial change, but please, whenever you change a file, modify the copyright notice to cover the current year. So, in this example, change 2017-2025 to 2017-2026.

@MilanTyagi2004
Copy link
Contributor Author

Hi @guusdk,

I have implemented the new changes, but before pushing the code to Git, I want to verify that everything is working properly. However, I’m unable to find any button or link in Openfire where I can create a new node and then delete it, similar to what we have in the user Roster.

image

Technically, the implementation seems correct. Could you please guide me a bit or let me know how I can properly test whether the implementation is working?

@MilanTyagi2004
Copy link
Contributor Author

Hi @guusdk ,
I’ve implemented the changes and raised this PR.

Summary of updates:

Moved deletion logic from pubsub-node-subscribers.jsp to pubsub-node-subscriber-delete.jsp.
Added a confirmation step before deletion (instead of inline handling).
Introduced i18n entries for confirmation messages, including dynamic affiliation and node details.

The larger diff mainly comes from separating the deletion flow into a dedicated page and adding the confirmation layer.
Manually verified the flow over XMPP.

Let me know if anything should be adjusted.

@guusdk
Copy link
Member

guusdk commented Mar 24, 2026

This looks pretty good! While testing, I noticed one small issue. IF a node has no affiliates, the (empty) table renders incorrectly. I think the colspan is wrong or something. I don't think that's caused by your changes, but would you mind fixing this, as you're already touching these files?

image

@MilanTyagi2004
Copy link
Contributor Author

yes, you are right there is the UI Inconsistency after the affiliation deletion.
and i check with old code , this UI issue has nothing to do with new changes.

i will fix this , just need some time

@MilanTyagi2004
Copy link
Contributor Author

Hey guss the UI issue is solved.

image

@guusdk guusdk added the backport 5.0 on merge, GHA will generate a PR with these changes against 5.0 branch label Mar 25, 2026
@guusdk guusdk merged commit 067a832 into igniterealtime:main Mar 25, 2026
32 checks passed
@guusdk
Copy link
Member

guusdk commented Mar 25, 2026

Thank you!

@github-actions
Copy link

Backport failed for 5.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 5.0
git worktree add -d .worktree/backport-3215-to-5.0 origin/5.0
cd .worktree/backport-3215-to-5.0
git switch --create backport-3215-to-5.0
git cherry-pick -x dcb1a219feb6a6a2074122470b3fe2a0d5cf0c8b 0c670aaa8d88d2c6e55f26a567933f09cc99a6ad 3923e18765d4f5e77e7dc6e27cdf3b904bb88d5a 4456f70977da5debb4bdb5b5cd91e5e31881791a 3fc0a9b3a89060e46f372510a1dafb5cfc44951f 96bfa9490c23fed8e4f60bc8d1a02f32e39c3683 e31eb22be8212b934db92587e21751ad5c3682da 067a8324217ec02f701673621b86dbe82b7dc460

@guusdk guusdk removed the backport 5.0 on merge, GHA will generate a PR with these changes against 5.0 branch label Mar 25, 2026
@guusdk
Copy link
Member

guusdk commented Mar 25, 2026

Yeah, don't worry about it - it likely is a merge conflict with the i18n file. I'm not sure if we'll have another release for the 5.0 branch anyway, so not having a backport is not very important. The changes will be part of Openfire 5.1.0 regardless!

@MilanTyagi2004
Copy link
Contributor Author

ooh , i thought that ,i made some mistake
, now everything is clear

@MilanTyagi2004 MilanTyagi2004 deleted the OF-1387-fix-subscriber-delete-confirmation branch March 25, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants