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 a notice to the edit screen if ads.txt already exists on the server #19

Merged
merged 13 commits into from
Nov 25, 2019

Conversation

kkoppenhaver
Copy link
Contributor

Addresses #16

When the presence of an existing ads.txt is detected, display a notification on the edit screen.

screen shot 2018-02-22 at 3 09 28 pm

@helen
Copy link
Contributor

helen commented Mar 2, 2018

Hi! Thanks for the PR. A couple of notes from a first look:

  • All UI text needs to be run through (preferably escaped) translation functions, e.g. esc_html_e().
  • All-caps are less readable and should be done in CSS if absolutely necessary.
  • I'm not sure a file_exists() check on every load is a great idea for performance, or that $_SERVER['DOCUMENT_ROOT'] is guaranteed to be accurate. I don't currently have alternate suggestions, but something to think about :)

@kkoppenhaver kkoppenhaver force-pushed the 16-detect-existing-ads-txt branch from f017f53 to 5ce8416 Compare March 2, 2018 09:12
@kkoppenhaver
Copy link
Contributor Author

Re: the admin notice, those are good ideas. Cleaned it up a bit with that latest commit as well as added the translation and escaping:
screen shot 2018-03-02 at 2 58 02 am

As far as your third point, how do you feel about using something like wp_remote_get (or vip_safe_wp_remote_get) to get the data at http://exampleurl.com/adx.txt and comparing it to what the plugin expects the output to be? If the output matches, then ads.txt is being served from the plugin. If it doesn't, then there's likely a file already in place.

This call could be cached using transients to reduce the load on the server and would be accompanied by a Check Again link or something similar in the admin notice to re-perform this check if the user believes the file has been removed.

@tomjn
Copy link
Contributor

tomjn commented Apr 17, 2018

As far as your third point, how do you feel about using something like wp_remote_get (or vip_safe_wp_remote_get) to get the data at http://exampleurl.com/adx.txt and comparing it to what the plugin expects the output to be? If the output matches, then ads.txt is being served from the plugin. If it doesn't, then there's likely a file already in place.

In the case of VIP, you would be better checking for a VIP environment and if found, bypassing the check, as it wouldn't make a lot of sense to do in a VIP context

As for wp_remote_get, that sounds like something that would be more peformant as a JS based check

@kkoppenhaver
Copy link
Contributor Author

kkoppenhaver commented Aug 10, 2018

My only thought now that the check is in JS is whether we should attempt to cache the result in a cookie for some amount of time so it's not performing the JS check on every load of the ads.txt admin page.

@tomjn

@tomjn
Copy link
Contributor

tomjn commented Aug 10, 2018

It would only be doing the check on that page, so it's up to you

@kkoppenhaver
Copy link
Contributor Author

Alright then I think this is ready for another review. Not sure if that's @helen or who should be looking at that. Happy to make any further changes.

@tomjn
Copy link
Contributor

tomjn commented Sep 10, 2018

I've no further opinions on the matter

@jeffpaul jeffpaul added this to the 1.2 milestone Apr 25, 2019
@jeffpaul jeffpaul requested a review from helen August 8, 2019 17:00
inc/admin.php Outdated Show resolved Hide resolved
inc/admin.php Outdated Show resolved Hide resolved
inc/admin.php Outdated Show resolved Hide resolved
inc/admin.php Outdated Show resolved Hide resolved
js/admin.js Outdated Show resolved Hide resolved
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

requested some changes

@adamsilverstein
Copy link

Hey @kkoppenhaver - thank you for your contribution, this seems like a great user experience improvement for admins who may not realize the already have the ads.txt file.

I left some feedback on the PR (sorry for the long delay in getting back to you). Do you think you will have time to work on addressing my feedback sometime soon, or would like some assistance getting this over the finish line?

@kkoppenhaver
Copy link
Contributor Author

@adamsilverstein Thanks for the feedback. I should be able to take a look early next week.

@kkoppenhaver
Copy link
Contributor Author

@adamsilverstein I think that should take care of everything you've raised. I've run it through testing of various scenarios locally and the check is performing as expected.

js/admin.js Outdated Show resolved Hide resolved
js/admin.js Outdated Show resolved Hide resolved
js/admin.js Outdated Show resolved Hide resolved
js/admin.js Outdated Show resolved Hide resolved
js/admin.js Outdated Show resolved Hide resolved
js/admin.js Outdated Show resolved Hide resolved
@adamsilverstein adamsilverstein self-requested a review September 24, 2019 14:29
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

@kkoppenhaver This works great, thanks for keeping at it! I left a few small code suggestions and I would like to have @helen review the copy here, otherwise this is ready to go!

@adamsilverstein
Copy link

Current screenshot:

image

@adamsilverstein adamsilverstein self-requested a review September 24, 2019 14:49
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Fantastic.

Copy link
Contributor

@helen helen left a comment

Choose a reason for hiding this comment

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

Marking as needs changes but from me, just tweaking the notice language.

inc/admin.php Outdated

<div class="notice notice-error adstxt-notice existing-adstxt" style="display: none;">
<p><strong><?php echo esc_html_e( 'Existing Ads.txt file found', 'ads-txt' ); ?></strong></p>
<p><?php echo esc_html_e( 'You will need to rename or remove the existing ads.txt file before you will be able to see any changes you make to ads.txt inside the WordPress admin.', 'ads-txt' ); ?></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like for this to say something about why there's a difference between an actual file and what this plugin is doing, because I don't think that's very clear to most people. I'll think on exact language.

Choose a reason for hiding this comment

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

How about something like:

We detected a hard coded ads.txt file on your site. This file takes precedence over the plugin, preventing it from working correctly. Please remove this file to use Ads-txt.

@jeffpaul jeffpaul mentioned this pull request Nov 25, 2019
17 tasks
Copy link
Contributor

@helen helen left a comment

Choose a reason for hiding this comment

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

I added a spinner for visual feedback and massaged the messaging and I think we're good to go for this release. We will probably want to rethink the actual detection approach here (again) at some point given #26 or maybe use a specific query arg name that we detect but I don't want to let this languish forever :)

@helen helen merged commit c345d49 into 10up:master Nov 25, 2019
helen added a commit that referenced this pull request Nov 25, 2019
Because I didn't notice the branch on #19 :(
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