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

Fix gravity count when FTL fails to return this value #3160

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

rdwebdesign
Copy link
Member

@rdwebdesign rdwebdesign commented Oct 24, 2024

What does this PR aim to accomplish?

Fixes the -2 Domains on the Lists issue.

How does this PR accomplish the above?

When FTL fails to count the number of domains on the lists, the value -2 is returned, but this value should not be printed.

This PR shows 0 in this case.

Link documentation PRs if any are needed to support this PR:

none


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

When FTL fails to count the number of domains on the lists, the value -2 is
returned, but this value should not be printed (it's just an error code).

Signed-off-by: RD WebDesign <[email protected]>
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.

I see were this is coming from, but I wonder if '-2' is not the better choose as it also indicates database failures/issues and not only empty gravity lists.

@rdwebdesign
Copy link
Member Author

In my opinion it is wrong to show -2 as the number of domains for the user.

If we need to indicate a database failure, then we need to show an error message somewhere. In this case, -2 is the value of DB_FAILED constant, not the domain count:

// Get number of domains in a specified table of the gravity database We return
// the constant DB_FAILED and log to FTL.log if we encounter any error
int gravityDB_count(const enum gravity_tables list)
{
    ...

https://github.com/pi-hole/FTL/blob/148e558fd17f1f8de88bab29b914fc14f8ef2423/src/database/gravity-db.c#L1074-L1077

and

https://github.com/pi-hole/FTL/blob/61a211f1c187206f5ff901afae657968114fde15/src/FTL.h#L96-L98

@yubiuser
Copy link
Member

Maybe we should show 0 and log an error to the message table as well? @DL6ER

@rdwebdesign
Copy link
Member Author

Maybe we should show 0 and log an error to the message table as well?

Yes.
The message can be shown in the messages table (if FTL generates the message), or we can simply reorder the code and add the message to the title attribute (see line 432) using javascript:

$(".small-box:has(#gravity_size)").attr("title", "ERROR MESSAGE HERE");

@DL6ER
Copy link
Member

DL6ER commented Oct 25, 2024

-2 typically means some really hard error on the database. Either the database does not exist (yet) or there is really some heavy error like mandatory tables missing. Even when the database is locked, it should still be possible to access it read-only to get these numbers. Is there any related issue ticket we could check for errors having been reported to FTL.log ?

FWIW my feeling is that -2 instead of 0 is more meaningful as it transports more info (as also said by @yubiuser above) but we'd need to (a) document this properly in the API docs (I didn't check if we do but I guess wo don't) and (b) handle this on the dashboard to show something more obvious to the user.


edit https://discourse.pi-hole.net/t/2-domains-on-blocklist-after-updating-pihole/73626/ is the context

@rdwebdesign
Copy link
Member Author

FWIW my feeling is that -2 instead of 0 is more meaningful

This is 100% true for the API, but not for the user interface.

From the UX point of view, -2 should be converted to an error message in the web interface.
A negative count is confusing and we have a lot of cases (since v5) showing users don't understand -2 as a valid number here.

@Th3M3
Copy link
Contributor

Th3M3 commented Oct 26, 2024

But would the user understand if and why it suddenly shows that there are 0 Domains on the lists ?

As I mentioned in my initial post on Discourse, pihole was still working and blocking queries as expected.
So in view of that 0 is misleading too, isn't it?

IMHO it would be most helpful, if

  • a message at the Pi-hole diagnosis system indicate what went wrong
  • the reason for showing -2 is documented somewhere (didn't searched the docs either tbh.)

@yubiuser
Copy link
Member

So in view of that 0 is misleading too, isn't it?

Maybe we could show ---- or Error + a tooltip suggesting to run gravity.

@rdwebdesign
Copy link
Member Author

I think I was able to achieve all goals.

Now in case of an error:

  • no more confusion (the count won't show as -2 domains);
  • no misleading 0 domains;
  • the box will show an error message, containing the error code;
  • the tooltip text will show a message suggesting to update gravity.

image

I'm don't think we need a message in diagnosis page, but in any case this should be done in FTL repo and is out of the scope of this PR.

- show an error message including the error code;
- change the title (tooltip) text

Signed-off-by: RD WebDesign <[email protected]>
@rdwebdesign rdwebdesign requested a review from a team October 27, 2024 21:57
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.

2024-10-28_15-23

@rdwebdesign rdwebdesign merged commit 74a5367 into development Oct 28, 2024
11 checks passed
@rdwebdesign rdwebdesign deleted the fix/gravity_no_count branch October 28, 2024 15:34
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.

4 participants