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

Adding in tests and report creation button #1

Open
wants to merge 12 commits into
base: plat-59
Choose a base branch
from
Open

Adding in tests and report creation button #1

wants to merge 12 commits into from

Conversation

kmayo-ss
Copy link
Owner

No description provided.

@@ -0,0 +1,34 @@
<?php

class ExternalLinks extends FunctionalTest {

Choose a reason for hiding this comment

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

This should be named "ExternalLinksTest"

@tractorcow
Copy link

A problem I've come across is that CheckExternalLinksJob will always run every link on every page when 'process' is called. This means that if the execution fails, each time this job restarts it will start execution from the beginning.

If you look at the version I started working on, the job will process one Page object at a time, which means that any errors between pages are recoverable in case of error. https://github.com/tractorcow/externallinks/blob/pulls/damo/code/jobs/CheckExternalLinksJob.php#L92



public function createQueuedReport() {
if (!Permission::check('ADMIN')) return;

Choose a reason for hiding this comment

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

There are users that aren't admins but are able to view reports.

Choose a reason for hiding this comment

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

I think this class is fine overall though. :) Maybe it just needs a new permission (can check links?) and only display that button in the CMS if the user has that permission.

@tractorcow
Copy link

Overall it's good work. :) Just needs a few tweaks IMO and it's there.

Do you mind if we fork this to ss-labs once this is merged?

tractorcow pushed a commit to tractorcow/externallinks that referenced this pull request Nov 19, 2015
API Add description for response code to report
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.

2 participants