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

1119 Privacy - Query Page and Dom.storage #2026

Merged
merged 4 commits into from
Feb 10, 2022

Conversation

chrismiceli
Copy link
Contributor

@chrismiceli chrismiceli commented Dec 25, 2021

handle localStorage being null

Signed-off-by: Chris Miceli chrismiceli@outlook.com

By submitting this pull request, I confirm the following: {please fill any appropriate checkboxes, e.g: [X]}

{Please ensure that your pull request is for the 'devel' branch!}

  • 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?:
Addresses #1119

How does this PR accomplish the above?:
This PR defaults all storage access to as if the item wasn't present to allow execution of scripts to continue. Note that I left some cases where a user interacts with a setting so that there is an indication the preference/setting wasn't persisted.

What documentation changes (if any) are needed to support this PR?:
None, unless we want to officially document when storage is disabled.

@chrismiceli chrismiceli force-pushed the issue/1119 branch 2 times, most recently from 5f9b64b to 99c7182 Compare December 25, 2021 04:42
handle localStorage being null

Signed-off-by: Chris Miceli <chrismiceli@outlook.com>
localStorage.setItem("barchart_chkbox", true);
if (localStorage) {
localStorage.setItem("barchart_chkbox", true);
}
}

bargraphs.click(function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that I left the click functions for settings a storage object unguarded so there is a console error message to indicate the saving failed. I guarded initialization routines that set storage to prevent further code running during initialization from getting blocked.

@rdwebdesign rdwebdesign linked an issue Jan 11, 2022 that may be closed by this pull request
3 tasks
PromoFaux
PromoFaux previously approved these changes Feb 3, 2022
@PromoFaux PromoFaux requested a review from a team February 3, 2022 18:08
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.

What's about L233 inutils.js?

function stateSaveCallback(itemName, data) {
  localStorage.setItem(itemName, JSON.stringify(data));
}

I tried your PR locally and I wasn't able to display the query log in FF with dom.stroage=disabled. I got errors about stateSaveCallback

@chrismiceli
Copy link
Contributor Author

What's about L233 inutils.js?

function stateSaveCallback(itemName, data) {
  localStorage.setItem(itemName, JSON.stringify(data));
}

I tried your PR locally and I wasn't able to display the query log in FF with dom.stroage=disabled. I got errors about stateSaveCallback

I intentionally left the modification methods untouched so that the user could see an error in the console about why their settings are not being persisted in storage. I could log a more specific message in those cases.

Let me see why query log isn't loading.

implement memory storage for functionality in case of no storage

Signed-off-by: Chris Miceli <chrismiceli@outlook.com>
@chrismiceli
Copy link
Contributor Author

What's about L233 inutils.js?

function stateSaveCallback(itemName, data) {
  localStorage.setItem(itemName, JSON.stringify(data));
}

I tried your PR locally and I wasn't able to display the query log in FF with dom.stroage=disabled. I got errors about stateSaveCallback

I intentionally left the modification methods untouched so that the user could see an error in the console about why their settings are not being persisted in storage. I could log a more specific message in those cases.

Let me see why query log isn't loading.

I pushed a commit @yubiuser to address the query log not loading. I now use a memory storage mechanism which won't survive page loads but enables callbacks to trigger correctly.

be explicit for undefined

Signed-off-by: Chris Miceli <chrismiceli@outlook.com>
fix else

Signed-off-by: Chris Miceli <chrismiceli@outlook.com>
@rdwebdesign
Copy link
Member

Of course. This is what happens when I rush :(. I pushed a commit to resolve.

It happens to anyone. No need to rush.
Take your time and test the code, if needed.

@yubiuser
Copy link
Member

yubiuser commented Feb 6, 2022

Confirm query log is working now.

@yubiuser yubiuser merged commit f727fb7 into pi-hole:devel Feb 10, 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-14-web-v5-11-and-core-v5-9-released/53529/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.

Privacy - Query Page and Dom.storage
5 participants