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

add link to swap between blocked and all client queries #2129

Merged
merged 1 commit into from
Mar 11, 2022
Merged

add link to swap between blocked and all client queries #2129

merged 1 commit into from
Mar 11, 2022

Conversation

stuXORstu
Copy link
Contributor

Signed-off-by: stuXORstu stuxorstu@gmail.com

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.
  • 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)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

Inserts a quick handy link to swap between blocked and all queries for clients. Seemed like a handy thing to have since when troubleshooting potential blocks I end up first looking at blocked queries, and then if I can't spot anything want to look at all queries to confirm if recent activity is not being blocked - so having a quick link to jump between those two states seemed useful to me, particularly when using a mobile device and not wanting to mess about with multiple tabs

How does this PR accomplish the above?:

Modification of the existing code that shows queries types and provides context to the user on the queries.php page

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

None - it is fairly straightforward (hopefully!!)

@PromoFaux PromoFaux requested a review from a team March 5, 2022 16:08
PromoFaux
PromoFaux previously approved these changes Mar 5, 2022
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.

I've tested and it does exactly what it says on the tin. I like the idea, but I'd like a second opinion from @pi-hole/web-maintainers first, in case I'm missing a reason why not to include this...

Copy link
Member

@yubiuser yubiuser 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 your PR.

I had spend some time to simplify the logic here. Most importantly: we have only a single place where we use type at all - it's the dashboard link in the "Top Clients (blocked) only)". The request will only work, if also a client is submitted
https://github.com/pi-hole/AdminLTE/blob/b57ff6aeec72e99f0226e339fcaf55b3567ee6ae/api_FTL.php#L297-L300

Therefore I think the logic can be a bit simplified:

  1. don't add if(isset($_GET["type"]) && $_GET["type"] === "blocked") to if($setupVars["API_QUERY_LOG_SHOW"] === "all")
  2. remove line 33-35
  3. add blocked in $showing .= " queries for client ".htmlentities($_GET["client"]);

@stuXORstu
Copy link
Contributor Author

Thanks for your advice and clarifying the logic too. I had initially added some cautious additional logic because I was not sure whether there might have been some ways to exercise params that I could not see from my simple pi-hole setup. If there is really no point to that as you say, then I've removed the additional complexity as you suggest.

@stuXORstu stuXORstu reopened this Mar 10, 2022
@stuXORstu stuXORstu requested a review from yubiuser March 10, 2022 19:29
@yubiuser
Copy link
Member

I didn't meant to be so permissive ;-)
See my comments. Otherwise PR looks good.

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Bildschirmfoto zu 2022-03-10 21-01-39
Bildschirmfoto zu 2022-03-10 21-01-45

queries.php Outdated Show resolved Hide resolved
queries.php Outdated Show resolved Hide resolved
queries.php Outdated Show resolved Hide resolved
queries.php Outdated Show resolved Hide resolved
@rdwebdesign
Copy link
Member

If all requested changes are applied, the code will end up with 2 if statements with the same expression:

    if (isset($_GET["type"]) && $_GET["type"] === "blocked")
    {
        $showing .= " blocked";
    }

    // Add switch between showing all queries and blocked only
    $showing .= " queries for client ".htmlentities($_GET["client"]).", <a href=\"?client=".htmlentities($_GET["client"]);

    if (isset($_GET["type"]) && $_GET["type"] === "blocked") {
        $showing .= "\">show all</a>";
    } else {
        $showing .= "&type=blocked\">show blocked only</a>";
    }

I think using only one if will make the code more readable:

    if (isset($_GET["type"]) && $_GET["type"] === "blocked") {
        // Show Blocked queries for this client + link to all
        $showing .= ' blocked queries for client '.htmlentities($_GET["client"]);
        $showing .= ', <a href="?client='.htmlentities($_GET["client"]).'">show all</a>';
    } else {
        // Show All queries for this client + link to show only blocked
        $showing .= ' all queries for client '.htmlentities($_GET["client"]);
        $showing .= ', <a href="?client='.htmlentities($_GET["client"]).'&type=blocked">show blocked only</a>';
    }

Co-authored-by: yubiuser <ckoenig@posteo.de>
Signed-off-by: stuXORstu <stuxorstu@gmail.com>
@stuXORstu
Copy link
Contributor Author

Yeah that is more readable than trying to fit into the smaller variable string concatenation at a time. Thanks, I've stitched it all together in another commit.

@stuXORstu stuXORstu requested a review from yubiuser March 11, 2022 01:20
@yubiuser yubiuser merged commit fa2df45 into pi-hole:devel Mar 11, 2022
@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-ftl-v5-15-web-v5-12-and-core-v5-10-released/54987/1

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.

5 participants