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

Broken Link Checker #81

Closed
Tracked by #1704
svenseeberg opened this issue Nov 17, 2018 · 10 comments · Fixed by #812
Closed
Tracked by #1704

Broken Link Checker #81

svenseeberg opened this issue Nov 17, 2018 · 10 comments · Fixed by #812
Assignees
Labels
‼️ prio: high Needs to be resolved ASAP. 💡 feature New feature or request

Comments

@svenseeberg
Copy link
Member

svenseeberg commented Nov 17, 2018

Verwalter should be able to automatically check all links in the content. All links that return a 200 or 301/302 are ok.

@benginoe benginoe self-assigned this Mar 11, 2019
@benginoe
Copy link
Member

What should be replied if we get any connection error which appears in try-except-manner (ConnectionError - at the moment i give back a HTTP: 503)?

I am giving back a list with link, http_code, datetime, is this enough for you @sascha11110 to display the status?

And another question: to which folder in the cms-subdirectory should i commit this class?

@svenseeberg svenseeberg added this to the Additional Features milestone Jun 7, 2019
@benginoe
Copy link
Member

benginoe commented Jul 4, 2019

Verwalter should have the possibility to download a excel table with all broken links

@benginoe
Copy link
Member

There should be the possibility to inform more then one person via mail.

@benginoe
Copy link
Member

There should also be the possibility to let the algorithm check on image tags and if the media file is still available, it should be visible in the view which type of content was checked.

@timobrembeck timobrembeck added 💡 feature New feature or request ⁉️ prio: low Not urgent, can be resolved in the distant future. labels Sep 25, 2019
@ulliholtgrave ulliholtgrave self-assigned this Mar 10, 2021
@svenseeberg svenseeberg added ‼️ prio: high Needs to be resolved ASAP. and removed ⁉️ prio: low Not urgent, can be resolved in the distant future. labels Apr 21, 2021
@ulliholtgrave ulliholtgrave removed their assignment Apr 21, 2021
@melegiul melegiul self-assigned this Apr 28, 2021
@melegiul
Copy link
Contributor

melegiul commented May 5, 2021

I found a django app for this feature, that could be helpful: django-linkcheck.
This app can crawl through the specified models to check for broken links.

broken-link-screen

Pros:

  • Saves us some work
  • Logic seems to be very efficient: uses post_save signals and assigns tasks to threads.
  • I like the nested lists feature.

Cons:

  • One model of the app uses a NullBooleanField, which is deprecated. There is a open PR, but it couldn't be merged yet.
  • The default location is the admin site and the app does not use class based views, so customization will get more difficult. I would need to replace the urls, views and templates, but still use the linkcheck logic under the hood.

Do you want to give the app a try or should we re-engineer something similar?

@timobrembeck
Copy link
Member

I'ts a pity that this is optimized for Django admin, but I guess the complicated part is the link-checking itself which we could definitely use. The link checking seems a sufficient complex topic to me that we are willing to make some concessions. If we want a 100% perfectly customized solution for us, we can also implement this in the distant future when there are no other important issues anymore 🙂

I think the NullBooleanField isn't a problem, since "deprecated" just means it will be removed in the next major version (Django 4.0)... but we won't upgrade to Django 4 until there is the LTS release 4.2 which is planned to be released in April 2023... so hopefully until then, the PR gets merged :D

One thing to test before we get too invested: Do the internals of this app also work when Django admin is not in INSTALLED_APPS? (Because Django admin is only activated in DEBUG mode)

@timobrembeck
Copy link
Member

Regarding a mock-up of the broken link checker UI: This is the current wordpress UI:

Working Links
Screenshot_2021-05-05 Fehlerhafte Links anzeigen ‹ Testumgebung App — WordPress(2)
Broken Links
Screenshot_2021-05-05 Fehlerhafte Links anzeigen ‹ Testumgebung App — WordPress
Obviously, you don't have to copy the design 1:1 since it's not perfect, but use it as a rough guideline.

@melegiul
Copy link
Contributor

melegiul commented May 5, 2021

One thing to test before we get too invested: Do the internals of this app also work when Django admin is not in INSTALLED_APPS? (Because Django admin is only activated in DEBUG mode)

I think it should work. I just remove admin from installed apps and then it looks like this:
Screenshot from 2021-05-05 19-12-17
Seems like the only dependency to admin was {% extends "admin/change_list.html" %} in this file
But we need to remove the app filer as well when the admin app is not set.

melegiul added a commit that referenced this issue May 5, 2021
@timobrembeck
Copy link
Member

timobrembeck commented May 5, 2021

I think it should work.

Cool! 👍

But we need to remove the app filer as well when the admin app is not set.

Yes, I think the new media library which is currently being developed by @jnugh and @ulliholtgrave works independently from django-filer, right?

@ulliholtgrave
Copy link
Member

Yep, never used it

melegiul added a commit that referenced this issue May 7, 2021
melegiul added a commit that referenced this issue May 11, 2021
Added linkcheck list view and basic template
Annotate links with the title of associated content
Adds pagination to templates
Disable linkcheck listeners when loading fixtures and during tests

Fixes:#81

Co-authored-by: Timo Ludwig <ludwig@integreat-app.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
‼️ prio: high Needs to be resolved ASAP. 💡 feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants