Move croquemort features from udata to udata-croquemort v2 #1110

abulte · 2017-09-07T07:17:07Z

Replace #1099

Depends on opendatateam/udata-croquemort#2 (comment) — see there for more info.

abulte · 2017-09-13T08:34:49Z

@noirbizarre I'd be happy to have your feedback at this point, especially about the XXXs in the code. Once I know how to best organize the CheckDatasetResource.get logic, I should be able to try and test it.

abulte · 2017-09-13T14:26:44Z

@noirbizarre proposed refactor (cf previous comment) in 63f9fb8 and tests added.

davidbgk · 2017-09-13T16:04:33Z

udata/linkchecker/backends.py

+
+log = logging.getLogger(__name__)
+
+DEFAULT_LINKCHECKER = 'croquemort'


I think that croquemort should not be referenced here but comes from udata-croquemort's settings.

Maybe introduce a DEFAULT_LINKCHECKER configuration variable with no_check by default and has to be set to croquemort when you install udata-croquemort?

Or something like LINK_CHECKER = 'path.to.DummyChecker' by default, I kinda like the django-like Python path in settings.

I pushed something more coherent IMHO. no_check is now an entrypoint provided linkchecker in udata (there is your django-like path ;-)). LINKCHECKING_DEFAULT_LINKCHECKER is by default the key to this linkchecker. It will be replaced by croquemort if you install udata-croquemort and want it to be the default one (or whatever linkchecker you want).

davidbgk · 2017-09-18T13:38:48Z

setup.py

@@ -132,6 +132,9 @@ def pip(filename):
            'ods = udata.harvest.backends.ods:OdsHarvester',
            'ckan = udata.harvest.backends.ckan:CkanBackend',
            'dcat = udata.harvest.backends.dcat:DcatBackend',
+        ],
+        'udata.linkcheckers': [
+            'no_check = udata.linkchecker.backends:NoCheckLinchecker',


(missing k)

davidbgk · 2017-09-18T13:40:36Z

udata/core/dataset/api.py

+    def get(self, dataset, rid):
+        '''Checks that a resource's URL exists and returns metadata.'''
+        resource = self.get_resource_or_404(dataset, rid)
+        result, status = check_resource(resource)


return check_resource ?

davidbgk · 2017-09-18T13:41:32Z

udata/core/dataset/models.py

@@ -366,21 +349,13 @@ def check_availability(self):
        Return a list of booleans.
        """
        # Only check remote resources.
+        # XXX is this enough? from the frontend we check every type


AFAIK, it is only in use within the admin to get global stats. To be checked.

davidbgk · 2017-09-18T13:42:14Z

udata/linkchecker/backends.py

+ENTRYPOINT = 'udata.linkcheckers'
+
+
+class NoCheckLinchecker(object):


NoCheckLinkChecker (missing k)

davidbgk · 2017-09-18T13:43:01Z

udata/linkchecker/backends.py

+
+def get(name):
+    '''Get a linkchecker given its name or fallback on default'''
+    all_lc = get_all()


Not fond of shortened variables names but YMMV

yes, linkcheckers or link_checkers might be more explicit

davidbgk · 2017-09-18T13:44:28Z

udata/linkchecker/checker.py

+    # store the check result in the resource's extras
+    resource.extras.update(_get_check_keys(result))
+    resource.save()
+    return result, result.get('check:status')


Is that pertinent to keep the status in result dict?

I think it can be more straightforward on frontend.

davidbgk · 2017-09-18T13:45:10Z

udata/linkchecker/checker.py

+    the `resource.extras['check:checker']` attribute with a key that points
+    to a valid `udata.linkcheckers` entrypoint. If not set, it will
+    fallback on croquemort. If set to `no_check` it will assume the resource
+    is available via a dummy linkchecker.


Add a line about the fact that it returns a flask-like response (dict, status)

davidbgk · 2017-09-18T13:46:46Z

udata/settings.py

+    LINKCHECKING_ENABLED = True
+    LINKCHECKING_IGNORE_DOMAINS = []
+    LINKCHECKING_CACHE_DURATION = 60 * 5  # in seconds
+    LINKCHECKING_DEFAULT_LINKCHECKER = 'no_check'


I don't get why we don't put the path here too? udata.linkchecker.backends:NoCheckLinchecker

This is the endpoint name

Are these settings documented somewhere?

davidbgk · 2017-09-18T13:47:04Z

udata/settings.py

@@ -184,6 +187,7 @@ class Defaults(object):
    #     'RESOURCE_DOWNLOAD': , # Demo = 5, Prod = ?
    #     'RESOURCE_REDIRECT': , # Demo = 6, Prod = ?
    # }
+    # TODO


noirbizarre · 2017-09-18T16:54:46Z

udata/core/dataset/api.py

+    def get(self, dataset, rid):
+        '''Checks that a resource's URL exists and returns metadata.'''
+        resource = self.get_resource_or_404(dataset, rid)
+        result, status = check_resource(resource)


noirbizarre · 2017-09-18T16:55:51Z

udata/core/dataset/models.py

-        return self.check_availability(group=None)
+        Non checked resources are presumed available.
+        '''
+        return self.extras.get('check:available', True)


Available by default ? Why not the opposite ? Or 'unknown' by default ?

This is used by dataset.quality.has_unavailable_resources. I think we do not want to flag some datasets as "bad quality" just because we did not check their resources. (It was already done like that).

Nop, but I think we can safely set it to unknown (I saw this possibility in this PR). This is more accurate I think

noirbizarre · 2017-09-18T16:57:48Z

udata/linkchecker/backends.py

+
+def get(name):
+    '''Get a linkchecker given its name or fallback on default'''
+    all_lc = get_all()


yes, linkcheckers or link_checkers might be more explicit

noirbizarre · 2017-09-18T16:58:22Z

udata/linkchecker/backends.py

+    lc = all_lc.get(name)
+    if not lc:
+        default_lc = current_app.config.get(
+                                'LINKCHECKING_DEFAULT_LINKCHECKER')


LINKCHECKER_DEFAULT or DEFAULT_LINKCHECKER ?

I think it has to start by LINKCHECKING_ to stay coherent with the other settings. And I used the LINKCHECKING_ prefix and not LINKCHECKER_ because LINKCHECKER_ENABLED does not really make sense since we can have multiple link checkers, LINKCHECKING_ENABLED is better IMHO.

noirbizarre · 2017-09-18T16:59:32Z

udata/linkchecker/backends.py

+    )
+
+
+def _ep_to_kv(entrypoint):


Duplicate of https://github.com/opendatateam/udata/blob/master/udata/harvest/backends/__init__.py#L17-L26
Should be factorized

noirbizarre · 2017-09-18T17:07:10Z

udata/linkchecker/checker.py

+    cached_check = get_cache(resource)
+    if cached_check:
+        return cached_check
+    linkchecker_type = resource.extras.get('check:checker', 'croquemort')


The default value should be configurable, especially if the croquemort implementation is in an optionnal extension.
Other alternative: handle the fact that there may not be a default link checker (and provide an unkown status)

You're right. And this is useless since I implemented to no_check fallback and LINKCHECKING_DEFAULT_LINKCHECKER.

noirbizarre · 2017-09-18T17:07:27Z

udata/linkchecker/checker.py

+    elif result.get('check:error'):
+        return {'error': result['check:error']}, 500
+    elif not result.get('check:status'):
+        return {'error': 'No status in response from linkchecker'}, 503


error or unkown ?

Same NB as above.

noirbizarre · 2017-09-18T17:08:16Z

udata/settings.py

+    LINKCHECKING_ENABLED = True
+    LINKCHECKING_IGNORE_DOMAINS = []
+    LINKCHECKING_CACHE_DURATION = 60 * 5  # in seconds
+    LINKCHECKING_DEFAULT_LINKCHECKER = 'no_check'


This is the endpoint name

noirbizarre · 2017-09-18T17:08:48Z

udata/settings.py

+    LINKCHECKING_CACHE_DURATION = 60 * 5  # in seconds
+    LINKCHECKING_DEFAULT_LINKCHECKER = 'no_check'
+
+    # `udata-croquemort` settings


We can drop these lines I think

noirbizarre · 2017-09-18T17:10:30Z

udata/templates/macros/metadata.html

@@ -46,8 +46,8 @@
 />
 {% endif %}

-<meta name="check-urls" content="{{ 'true' if config.CROQUEMORT else 'false' }}" />
-<meta name="check-urls-ignore" content="{{ config.CROQUEMORT_IGNORE|tojson|urlencode }}" />
+<meta name="check-urls" content="{{ 'true' if config.LINKCHECKING_ENABLED else 'false' }}" />


Or <meta name="check-urls" content="{{ config.LINKCHECKING_ENABLED|to_json }}" />
Works for other booleans.

noirbizarre

Nice start

davidbgk · 2017-09-25T12:36:27Z

js/front/dataset/index.js

                        }
-                    });
+                    })
+                    .catch(error => console.log('Something went wrong with the linkchecker', error));


Don't we want to put a format-label-unchecked or something like that in this case?

Yes, could come in handy 👍

davidbgk · 2017-09-25T12:39:06Z

udata/core/dataset/models.py

-    @property
-    def is_available(self):
-        return self.check_availability(group=None)
+        NB: `unknown` will evaluate to True in the aggregate checks using


It kind of is :-/ I hesitated with None but the advantage of this solution is that we don't need to modify the aggregate checks (all([])).

davidbgk · 2017-09-25T12:39:59Z

udata/core/user/models.py

@@ -135,11 +135,13 @@ def resources_availability(self):
                *[org.check_availability() for org in self.organizations]
            )
        )
+        # Filter out the unknown
+        availabilities = [a for a in availabilities if type(a) is bool]


Why not using isinstance?

I like that when checking for a basic type because it reads as en English sentence.

davidbgk · 2017-09-25T12:42:12Z

udata/settings.py

+    LINKCHECKING_ENABLED = True
+    LINKCHECKING_IGNORE_DOMAINS = []
+    LINKCHECKING_CACHE_DURATION = 60 * 5  # in seconds
+    LINKCHECKING_DEFAULT_LINKCHECKER = 'no_check'


Are these settings documented somewhere?

abulte · 2017-09-25T14:05:00Z

@davidbgk I can't reply on your comment on udata/settings.py (don't know why): nope they aren't documented, I have to do it 👍

noirbizarre · 2017-09-25T14:42:41Z

udata/core/user/models.py

@@ -135,11 +135,13 @@ def resources_availability(self):
                *[org.check_availability() for org in self.organizations]
            )
        )
+        # Filter out the unknown
+        availabilities = [a for a in availabilities if type(a) is bool]


abulte · 2017-09-26T08:28:13Z

Rebased on master.

abulte · 2017-09-27T14:44:22Z

@davidbgk is this OK for you?

davidbgk

👍 Good work!

davidbgk · 2017-09-27T16:02:05Z

js/front/dataset/index.js

+                    this.$api.get(checkurl)
+                    .then((res) => {
+                        const status = res['check:status'];
+                        if (status >= 200 && status < 400) {


Thoughts for future: deal with redirection :)

davidbgk · 2017-09-27T16:03:32Z

udata/core/user/models.py

        if availabilities:
            # Trick will work because it's a sum() of booleans.
            return round(100. * sum(availabilities) / len(availabilities), 2)
        else:
-            return 0
+            return 100


Maybe explain why?

Croquemort specific stuff is removed from udata and will live in udata-croquemort. Linkchecking can now be specified on a resource level. Related change: default availability for a user's datasets is now 100% (vs 0%).

This was referenced Sep 7, 2017

Move croquemort features from udata to udata-croquemort opendatateam/udata-croquemort#2

Merged

[WIP] Move croquemort features from udata to udata-croquemort #1099

Closed

noirbizarre modified the milestone: 1.2.0 Sep 11, 2017

abulte force-pushed the feature/udata-croquemort-v2 branch from 2c25948 to 6acd50b Compare September 12, 2017 13:47

abulte changed the title ~~[WIP] Move croquemort features from udata to udata-croquemort v2~~ Move croquemort features from udata to udata-croquemort v2 Sep 13, 2017

abulte requested a review from a team September 13, 2017 15:46

davidbgk reviewed Sep 13, 2017

View reviewed changes

davidbgk suggested changes Sep 18, 2017

View reviewed changes

noirbizarre suggested changes Sep 18, 2017

View reviewed changes

noirbizarre reviewed Sep 19, 2017

View reviewed changes

davidbgk reviewed Sep 25, 2017

View reviewed changes

noirbizarre approved these changes Sep 25, 2017

View reviewed changes

abulte force-pushed the feature/udata-croquemort-v2 branch from 8d9b8dd to 5e386ac Compare September 26, 2017 08:26

abulte changed the base branch from dev to master September 26, 2017 08:27

abulte force-pushed the feature/udata-croquemort-v2 branch from 5e386ac to 35eb5a4 Compare September 27, 2017 13:02

davidbgk approved these changes Sep 27, 2017

View reviewed changes

abulte force-pushed the feature/udata-croquemort-v2 branch 5 times, most recently from 9547d04 to cbbed63 Compare October 4, 2017 11:40

abulte changed the base branch from master to feature/linkchecker October 4, 2017 11:40

abulte force-pushed the feature/udata-croquemort-v2 branch from cbbed63 to 959b57f Compare October 6, 2017 09:40

abulte force-pushed the feature/udata-croquemort-v2 branch from 959b57f to 986cb50 Compare October 18, 2017 08:21

abulte force-pushed the feature/udata-croquemort-v2 branch from 986cb50 to e720d02 Compare October 18, 2017 12:12

Abstract link checking through entrypoint

040f4a9

Croquemort specific stuff is removed from udata and will live in udata-croquemort. Linkchecking can now be specified on a resource level. Related change: default availability for a user's datasets is now 100% (vs 0%).

abulte force-pushed the feature/udata-croquemort-v2 branch from e720d02 to 040f4a9 Compare October 18, 2017 12:28

abulte merged commit ce56a0b into opendatateam:feature/linkchecker Oct 18, 2017

abulte deleted the feature/udata-croquemort-v2 branch October 18, 2017 12:34

abulte mentioned this pull request Oct 18, 2017

Abstract link checking through entrypoint (#1110) #1220

Merged


		log = logging.getLogger(__name__)

		DEFAULT_LINKCHECKER = 'croquemort'

		ENTRYPOINT = 'udata.linkcheckers'


		class NoCheckLinchecker(object):

Move croquemort features from udata to udata-croquemort v2 #1110

Move croquemort features from udata to udata-croquemort v2 #1110

Conversation

abulte commented Sep 7, 2017 • edited

abulte commented Sep 13, 2017 • edited

abulte commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abulte Sep 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noirbizarre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abulte commented Sep 25, 2017

Choose a reason for hiding this comment

abulte commented Sep 26, 2017

abulte commented Sep 27, 2017

davidbgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abulte commented Sep 7, 2017 •

edited

abulte commented Sep 13, 2017 •

edited

abulte Sep 14, 2017 •

edited