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

Make terms content an easily customizable markdown content #1285

Merged
merged 2 commits into from Dec 4, 2017
Merged

Make terms content an easily customizable markdown content #1285

merged 2 commits into from Dec 4, 2017

Conversation

noirbizarre
Copy link
Contributor

@noirbizarre noirbizarre commented Nov 29, 2017

This PR externalize the terms handling.
udata now expect a markdown content into a remote or local file.
The target file is defined by the SITE_TERMS setting.
It default on an embedded markdown content which indicate to the administrator it needs to define terms.

NB: this prevent mailto: links to render as links

**default**: `'default'`

The site terms in markdown. It can be either an URL to a markdown content or a local path.
If this is an URL, the content is downloaded on the first terms page display and cached.
Copy link
Contributor

Choose a reason for hiding this comment

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

How long is it cached? Is it related to another udata setting? Is it done by following HTTP cache instructions?

Copy link
Contributor

Choose a reason for hiding this comment

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

udata/cgu.md Outdated
@@ -0,0 +1,6 @@
# Generic terms and conditions
Copy link
Contributor

Choose a reason for hiding this comment

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

cgu.md is based on a french naming (Conditions Générales d'Utilisation). Would not it be more consistent to name it terms.md?

Also, should we rather return a 404 page on the terms page if nothing has been specified? I'd be more comfortable to return nothing rather than a content which cannot be used as is.

Something like udata doctor could be used to list recommended changes to improve udata settings beyond its defaults.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for terms.md. Not sure about the 404.

@thom4parisot
Copy link
Contributor

Nice work :-)

Is there part of your work which can be reused in the future to handle more content via remote static pages/markdown resources?

Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of SITE_TERMS settings name, seems ambiguous to me. Maybe SITE_TERMS_AND_CONDITIONS, or even SITE_TERMS_AND_CONDITIONS_MD_LOCATION but it's getting really long ;-)


**default**: `'default'`

The site terms in markdown. It can be either an URL to a markdown content or a local path.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/It can be either an URL to a markdown content or a local path./It can be either an URL or a local path to a markdown content./

Copy link
Contributor

Choose a reason for hiding this comment

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

s/terms/terms and conditions/

udata/cgu.md Outdated
@@ -0,0 +1,6 @@
# Generic terms and conditions
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for terms.md. Not sure about the 404.

udata/cgu.md Outdated

Be excellent to each other.

You see this content because you don't have provided a proper `SITE_TERMS`
Copy link
Contributor

Choose a reason for hiding this comment

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

s/SITE_TERMS/SITE_TERMS setting/

**default**: `'default'`

The site terms in markdown. It can be either an URL to a markdown content or a local path.
If this is an URL, the content is downloaded on the first terms page display and cached.
Copy link
Contributor

Choose a reason for hiding this comment

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

@noirbizarre
Copy link
Contributor Author

@oncletom I think some of this work can/may be reusable for other content but I prefer to factorize when it's required in order to have all parameters.

@abulte I switched to SITE_TERMS_LOCATION as a compromise, is it OK for you ?

About udata doctor I think this is a great idea of PR 👍
For the 404, given the fact that terms URL is exported in the sitemap, it may require a little refactoring. In another PR ?

@abulte
Copy link
Contributor

abulte commented Dec 4, 2017

LGTM

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.

None yet

3 participants