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

Improve messages on the domain management pages #1420

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jun 2, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

See title

How does this PR accomplish the above?:

Test 1

Adding doubleclick.net on the blacklist shows:
Screenshot from 2020-06-02 11-32-51

Trying to add it again shows:
Screenshot from 2020-06-02 11-32-58

Test 2

Remove doubleclick.net from the blacklist and add doubleclick.net doubleclick.net`:
Screenshot from 2020-06-02 11-33-38

Again trying to add doubleclick.net doubleclick.net shows:
Screenshot from 2020-06-02 11-33-16

What documentation changes (if any) are needed to support this PR?:

None

@DL6ER DL6ER added this to the v5.1 milestone Jun 2, 2020
@DL6ER DL6ER requested a review from PromoFaux June 2, 2020 09:40
@yubiuser
Copy link
Member

yubiuser commented Jun 2, 2020

Warning #2 and #4 should not state "Success" and be not green.

@DL6ER
Copy link
Member Author

DL6ER commented Jun 2, 2020

@yubiuser So what instead? Orange warnings?

After all, what the user asked for happened: All domains are on the list. In my opinion, it's not important if they were there before, the user wants them to be on the list and they are.

@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/slight-bug-in-blacklist-uniqueness-constraint/33791/20

@yubiuser
Copy link
Member

yubiuser commented Jun 2, 2020

Mhh.. good point. I thought of "Warning" and orange.

But you're right the users want to see it on the list and you just telling them they are already. No further actions of the users is required - so no "real" warning.

P.S. Maybe I was distracted by the title of the PR stating "warning". In the end it is improved success message ;-)

@DL6ER DL6ER changed the title Improve warnings on the domain management pages Improve messages on the domain management pages Jun 2, 2020
@DL6ER
Copy link
Member Author

DL6ER commented Jun 2, 2020

Thanks, I edited the title to make it more clear.

@PromoFaux
Copy link
Member

Ignore my fat fingers forgetting how to type... If the list type has changed, is it possible to make the message say "[domain] has been moved from [type]list to [type]list", or is that too big an ask?

test com

@DL6ER
Copy link
Member Author

DL6ER commented Jun 3, 2020

How would you like to see this happening technically?

@PromoFaux
Copy link
Member

I'm not 100% sure. It was just something that stuck out when I was testing

@DL6ER
Copy link
Member Author

DL6ER commented Jun 4, 2020

@PromoFaux I checked how we could do this but the REPLACE action will silently replace the entry without us noticing. When we're in FTL, we can actually catch this using an update hook, however, I checked the PHP::SQLite3 docs and see that no such thing is available here.

So idea: Count number of domains for the same type? If this changed but the total number didn't, then the action was moving the domain from another type to here.

@DL6ER
Copy link
Member Author

DL6ER commented Jun 4, 2020

@PromoFaux Well, wait, we support adding two independent copies to the white- and blacklist so conversion does actually never happen?

Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

image
You're right, I was testing against an older version of the db schema

@PromoFaux PromoFaux merged commit 7aa79a7 into devel Jun 5, 2020
@PromoFaux PromoFaux deleted the tweak/improve_warnings branch June 5, 2020 18:17
@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
@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-5-1-released/35577/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants