add warning to settings page if SafeBrowsing is enabled without key

[stklcode]

Related to #104.

Google Safe Browsing with fallback API key is target for removal, so we inform the user that a custom API key must be provided to continue using this feature.
The warning will be displayed once after activation (upgrade) and always on the plugin's settings page.

[codecov-io]

Codecov Report

Merging #105 (b6a9eb1) into develop (b9c9afd) will decrease coverage by 1.48%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             develop     #105      +/-   ##
=============================================
- Coverage      39.43%   37.94%   -1.49%     
- Complexity       143      149       +6     
=============================================
  Files              5        5              
  Lines            535      556      +21     
=============================================
  Hits             211      211              
- Misses           324      345      +21     

Impacted Files	Coverage Δ	Complexity Δ
antivirus.php	0.00% <ø> (ø)	0.00 <0.00> (ø)
inc/class-antivirus.php	20.78% <0.00%> (-1.41%)	89.00 <1.00> (+6.00)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9c9afd...b6a9eb1. Read the comment docs.

[Zodiac1978]

Thanks for the PR!

I have tested the PR and it works as expected. The notice is there if the input field is empty. It re-appears if the content of the field is removed again.

But I have some remarks:

• $sb_key is used. Is there any reason why this is not more "speaking"? Like $safe_browsing_key?
• The notice is showing only once after activation and then only on the Antivirus settings page. Is this correct? We could show it everywhere and make it dismissible.
• The link to the documentation does not open in a new window/tab. I know there is much discussion about this. But maybe in this case it could be helpful? (Then we should add rel="noopener noreferrer" to the link.)
• I would recommend to add size="45" to the input field, so that the API key is completely visible in the text field.

[stklcode]

• $sb_key is used. Is there any reason why this is not more "speaking"? Like $safe_browsing_key?

Not really. Would have omitted the variable completely like ... empty( self::_get_option( 'safe_browsing_key' ) { , but unfortunately we declare compatibility with PHP <5.5, so that's not supported 🤷‍♂️ Only used once in the next line and will be gone with the next major update anyway, so I thought small scope / short name.

• The notice is showing only once after activation and then only on the Antivirus settings page. Is this correct? We could show it everywhere and make it dismissible.

Don't know if the behavior is "correct", but this was my intention. But yes, we could always show it.

• The link to the documentation does not open in a new window/tab. I know there is much discussion about this. But maybe in this case it could be helpful? (Then we should add rel="noopener noreferrer" to the link.)

I personally hate new windows enforcement, but in this case I'd guess about everybody would open it within a new window/tab.

• I would recommend to add size="45" to the input field, so that the API key is completely visible in the text field.

Probably little out of scope for this PR, but definitely a good point. (see #109)

[codecov-commenter]

Codecov Report

Merging #105 (e208fbf) into develop (fde1266) will decrease coverage by 1.25%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             develop     #105      +/-   ##
=============================================
- Coverage      39.00%   37.74%   -1.26%     
- Complexity       143      148       +5     
=============================================
  Files              5        5              
  Lines            541      559      +18     
=============================================
  Hits             211      211              
- Misses           330      348      +18     

Impacted Files	Coverage Δ	Complexity Δ
inc/class-antivirus.php	20.59% <0.00%> (-1.17%)	88.00 <1.00> (+5.00)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fde1266...e208fbf. Read the comment docs.

[Zodiac1978]

Only used once in the next line and will be gone with the next major update anyway, so I thought small scope / short name.

Yes, go for it! Not important in this case. Readability is not problematic here.

But yes, we could always show it.

I think it makes sense here. From the Plugin handbook:

Upgrade prompts, notices, alerts, and the like must be limited in scope and used sparingly, be that contextually or only on the plugin’s setting page. Site wide notices or embedded dashboard widgets must be dismissible or self-dismiss when resolved. Error messages and alerts must include information on how to resolve the situation, and remove themselves when completed.

As this will break in the next release we should show it sitewide, but with clear instructions on how to solve the situation. The message should remove if not necessary anymore or it was dismissed. Maybe always show on our settings page even if dismissed but not solved.

Maybe we should change the link text from "See official documentation." to "See official documentation from Google." (otherwise this could mean our documentation) and maybe we can add a link to our settings page to the notice to make it easier to resolve the issue fast.

[stklcode]

Done.

[Zodiac1978]

LGTM

Just one last note:

After disabling the safe browsing feature and save the settings the notice is still there (one click or reload later it is gone, but maybe this is confusing as the problem is solved after disabling the feature, so why still the notice?)

The dismiss of the notice is not persistent. It comes back on the next admin page. I am not sure if this correct, because the people need to fix this or not ...

We should in every case tell this change in a "upgrade notice" in the changelog/readme.txt.

Do we need to add, that the users can also disable the feature or is this silly?

The patch/branch does not contain the changes of #109 - just FYI. Not sure if this should be the case or not. Just wondered ...

[stklcode]

After disabling the safe browsing feature and save the settings the notice is still there (one click or reload later it is gone, but maybe this is confusing as the problem is solved after disabling the feature, so why still the notice?)

That's caused by suboptimal processing order. The initial global admin notice hook is done before the options are updated...

The dismiss of the notice is not persistent. It comes back on the next admin page. I am not sure if this correct, because the people need to fix this or not ...

Well, we had two variants now:

• always on the settings page + once after activation or upgrade
• always everywhere

There a number of ways inbetween. Remembering dismissal requires custom logic and an additional options flag, user meta or similar and that seems overdone for a "feature" that will be gone with the next update.

We could add something like after udpate, on the settings page and on the dashboard.

Do we need to add, that the users can also disable the feature or is this silly?

Implicated by "if you want to continue using this feature ...", isn't it?

The patch/branch does not contain the changes of #109 - just FYI. Not sure if this should be the case or not. Just wondered ...

That's fine, both changes will be visible after the merge.

[stklcode]

After disabling the safe browsing feature and save the settings the notice is still there (one click or reload later it is gone, but maybe this is confusing as the problem is solved after disabling the feature, so why still the notice?)

Fixed.

Slightly reworked the logic, s.t. the notice is always shown on the settings page and on other pages only for users who actually can manage options to not bother regular authors or editors.

[Zodiac1978]

Great work. Thanks for optimizing with my ideas.