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

Reverse logic of in/exlcuding queries from long-term database and rename queryType => statusType #2338

Merged
merged 2 commits into from
Sep 16, 2022

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Sep 8, 2022

What does this PR aim to accomplish?:

If users choose to deselect all "query" types (which are truly status types) on the long-term data page an PHP error occurs. The expected behavior is that no error occurs (and (very likely) no data is available).

  • How does this PR accomplish the above?:
  1. To match the documentation (https://docs.pi-hole.net/database/ftl/#supported-status-types), everything related to "queryType" was renamed to "statusType".
  2. Instead of querying the the database for the selected statusTypes, this PR reverses the logic and excludes the unchecked statusTypes. This solved the mentioned issue with all unchecked boxes, but additionally allows to retrieve queries with a statusType not explicitly defined. If, for example a new statusType was added by FTL we had no chance to see this unless we explicitly added it to the selection. Now it will always be 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)

  • 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 Sep 8, 2022
@yubiuser yubiuser requested a review from a team September 8, 2022 13:35
@yubiuser yubiuser marked this pull request as draft September 16, 2022 05:17
@yubiuser yubiuser force-pushed the no_error_empty_checkbox branch 2 times, most recently from f61ddc9 to 74aeedc Compare September 16, 2022 11:01
…me queryType => statusType

Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser changed the title Set query type to zero if no query type was selected for long-term data Reverse logic of in/exlcuding queries from long-term database and rena… …me queryType => statusType Sep 16, 2022
@yubiuser yubiuser marked this pull request as ready for review September 16, 2022 11:13
@yubiuser yubiuser changed the title Reverse logic of in/exlcuding queries from long-term database and rena… …me queryType => statusType Reverse logic of in/exlcuding queries from long-term database and rename queryType => statusType Sep 16, 2022
api_db.php Outdated Show resolved Hide resolved
Copy link
Member

@rdwebdesign rdwebdesign left a comment

Choose a reason for hiding this comment

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

You forgot to change it here.

api_db.php Outdated Show resolved Hide resolved
Co-authored-by: RD WebDesign <github@rdwebdesign.com.br>
Signed-off-by: yubiuser <ckoenig@posteo.de>
@yubiuser yubiuser merged commit 2d81c10 into devel Sep 16, 2022
@yubiuser yubiuser deleted the no_error_empty_checkbox branch September 16, 2022 18:21
@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-18-1-web-v5-15-1-and-core-v5-12-2-released/58022/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.

3 participants