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

improve link logging #1235

Merged
merged 1 commit into from
Apr 12, 2022
Merged

improve link logging #1235

merged 1 commit into from
Apr 12, 2022

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Apr 12, 2022

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

None

What's this PR do?

Miscellaneous improvements to the output of markdown_cleanup and link logging rule that I made when opening #1233:

  • now csv includes urls to github, studio, and ocw
  • new --limit option for the management command, useful for testing
  • copied/updated the classification from validate_urls to link_logging. Will probably delete validate_urls soon. (It's the old regexp based version of link_logging rule).

How should this be manually tested?

Run

docker-compose run --rm web ./manage.py markdown_cleanup link_logging -o links.csv --limit 100

and check that the links in the CSV work.

@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2022

Codecov Report

Merging #1235 (11280af) into master (d1a640e) will decrease coverage by 0.05%.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##           master    #1235      +/-   ##
==========================================
- Coverage   90.74%   90.68%   -0.06%     
==========================================
  Files         212      212              
  Lines        8839     8850      +11     
  Branches     1634     1635       +1     
==========================================
+ Hits         8021     8026       +5     
- Misses        709      715       +6     
  Partials      109      109              
Impacted Files Coverage Δ
...s/management/commands/markdown_cleaning/cleaner.py 68.83% <27.27%> (-6.93%) ⬇️
static/js/util/factories/websites.ts 98.48% <0.00%> (+3.03%) ⬆️

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 d1a640e...11280af. Read the comment docs.

@ChristopherChudzicki ChristopherChudzicki force-pushed the cc/link-logger branch 2 times, most recently from 73b6dd8 to 1d6d604 Compare April 12, 2022 18:20
@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review April 12, 2022 18:22
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Overall looks great, just one suggestion

Comment on lines +18 to +36
def get_ocw_url(content: WebsiteContent):
"""Return an ocw.mit.edu url to the given content."""
rootrel = get_rootrelative_url_from_content(content)
return f"https://ocw.mit.edu{rootrel}"


def get_studio_url(content: WebsiteContent):
"""Return an ocw-studio.odl.mit.edu url to the given content."""
site_name = content.website.name
if content.type == "sitemetadata":
return f"https://ocw-studio.odl.mit.edu/sites/{site_name}/type/metadata/"
return f"https://ocw-studio.odl.mit.edu/sites/{site_name}/type/page/edit/{content.text_id}/"


def get_github_url(content: WebsiteContent):
"""Return a github.mit.edu url to the given content."""
short_id = content.website.short_id
return f"https://github.mit.edu/mitocwcontent/{short_id}/tree/main/{content.dirpath}/{content.filename}.md"

Copy link
Member

Choose a reason for hiding this comment

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

Might be better to use django settings for the domains instead of hardcoding them. For example:
OCW_STUDIO_BASE_URL for the OCW Studio domain
GIT_DOMAIN for the github domain
OCW_STUDIO_LIVE_URL for ocwnext, though this isn't ocw.mit.edu on production (it's https://ocw-published.odl.mit.edu/ instead - maybe that should be changed?) but should lead to the same place.

@ChristopherChudzicki ChristopherChudzicki merged commit 31895c7 into master Apr 12, 2022
@odlbot odlbot mentioned this pull request Apr 12, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants