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

Tweak deleting message #2498

Merged
merged 2 commits into from
Jan 21, 2023
Merged

Tweak deleting message #2498

merged 2 commits into from
Jan 21, 2023

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Jan 17, 2023

What does this PR aim to accomplish?:

Be more specific what we delete from one of the tables. The current wording could be misinterpreted. See: https://discourse.pi-hole.net/t/database-locked-one-symptom-deleting-one-entry-in-pi-hole-diagnosis-triggers-deleetion-of-1616-invisible-messages-which-fails/60388/7?u=yubiuser

I changed the code to show meaningful information to the user when there is some (e.g. name of the group which is deleted) or only print a generic statement if not ("Deleting messag(s)").

In case of an error, the corresponding id is always shown.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@yubiuser yubiuser added the PR: Approval Required Open Pull Request, needs approval label Jan 17, 2023
@yubiuser yubiuser requested a review from a team January 17, 2023 06:18
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/database-locked-one-symptom-deleting-one-entry-in-pi-hole-diagnosis-triggers-deleetion-of-1616-invisible-messages-which-fails/60388/9

@saint-lascivious
Copy link

saint-lascivious commented Jan 19, 2023

From this Reddit thread, where this ended up confusing me for a different reason, and then giving me more questions:

What's the direct value of communicating this ID to the user? There's no way I would have even remembered the value if I didn't screenshot it and go looking for it consciously, and it's not clear what if anything one can do with that piece of information.

To be clear, with the proposed change, say I deleted multiple messages in a single session. Does that mean there would be N popups with "Deleted message/group/adlist/etc with ID: $FOO", where N == number of messages dismissed (incidentally the popup stacking when there's multiple messages can be kind of off-putting and prevent user interaction while you wait for the notification stack to disappear)?

A single notification displaying the number of messages deleted in total perhaps seems a little cleaner, but that's said in the context of not knowing what displaying the ID is intended to achieve.

Edit: If there's a TL;DR here it's basically "even when it's clear what the value is, it's not clear why it is".

@yubiuser
Copy link
Member Author

To be clear, with the proposed change, say I deleted multiple messages in a single session. Does that mean there would be N popups with "Deleted message/group/adlist/etc with ID: $FOO", where N == number of messages dismissed (incidentally the popup stacking when there's multiple messages can be kind of off-putting and prevent user interaction while you wait for the notification stack to disappear)?

If you delete multiple messages/clients/groups/.. there should be only one popup WIth "Deleting messages with ID: 1,2,3,4"


But I get your point. For the users it does not make sense to have this information. It does not translate to anything meaningful for them. I'll see if we can improve this or simplay stating "Deleting XXX...". In case of an error I would keep the ID. I think we can have more meaningful output for groups/clients/adlists, but maybe not "messages".... Let's see...

@yubiuser yubiuser added WIP and removed PR: Approval Required Open Pull Request, needs approval labels Jan 19, 2023
@jfb-pihole
Copy link
Member

jfb-pihole commented Jan 20, 2023

I am in favor of leaving the ID's in the message. This may be useful in diagnosing user problems in the future, and won't confuse any users. I would rather have it and rarely use it than rarely need it and not have it.

"Deleting messages with ID: 160, 167, 168" would be my preferred output.

@saint-lascivious
Copy link

I am in favor of leaving the ID's in the message. This may be useful in diagnosing user problems in the future

I would rather have it and rarely use it than rarely need it and not have it.

There's clearly an argument that it's useful information to have for potential debugging purposes, but I don't think that necessarily translates into being useful information to have in a confirmation notification that's user facing.

A (potentially quite large) list of IDs that might possibly be useful in the future is debug/log info.

@yubiuser
Copy link
Member Author

yubiuser commented Jan 20, 2023

I changed the code to show meaningful information to the user when there is some (e.g. name of the group which is deleted) or only print a generic statement if not ("Deleting messag(s)").

In case of an error, id is always shown.

Please try it with pihole checkout web tweak/messages

@yubiuser yubiuser added PR: Approval Required Open Pull Request, needs approval and removed WIP labels Jan 20, 2023
@yubiuser
Copy link
Member Author

Peek 2023-01-20 17-03

@saint-lascivious
Copy link

Looks fine over here.

Thanks yubi, for both your work, and being open to change. It's appreciated.

@yubiuser yubiuser merged commit 7a4be9e into devel Jan 21, 2023
@yubiuser yubiuser deleted the tweak/messages branch January 21, 2023 18:26
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-web-v5-18-2-and-core-v5-15-1-released/60695/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approval Required Open Pull Request, needs approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants