Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Add site pages #3

Closed
wants to merge 3 commits into from
Closed

Add site pages #3

wants to merge 3 commits into from

Conversation

GabeIsman
Copy link
Contributor

Very basic setup for site pages.

from scans.models import Site

class SitePage(Page):
site_url = models.CharField(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a good idea to duplicate this value across SitePage and Site, or use sync_site to keep the state synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you just worried about them getting out of sync? The title doesn't need to be duplicated, it's basically just for the friendliness of the scans output.

The url here is basically a foreign key, and could actually be implemented that way, which would solve the duplication problem, but would lead to issues if a site ever updated its url for some reason.

Alternatively we could duplicate none of this information and just have the scan command pull in the SitePage to get the url. However this approach seems cleaner to me. The scan command shouldn't need to know anything about the web app.

@garrettr
Copy link
Contributor

Introductory disclaimer: this is a bit tricky, and neither of us are very experienced with Wagtail or Django, so "best practices" are not obvious here and there is room for debate in how this gets implemented.


That being said, I do not think it is a good approach to create a SitePage for each Site. It seems to me that you are trying to accomplish two things here:

  1. Provide a convenient interface for admins to add/edit sites to be scanned
  2. Generate pages for each site (e.g. "results page for the Washington Post")

Note that 1. is already available through the default Django admin (available at /django-admin), not the Wagtail admin. So the functionality is available, although I would agree it would be a better experience for admins if it were available for the Wagtail admin. I think the best way to achieve that is probably through Wagtail's ModelAdmin.

For 2., the only reason I think it would be appropriate to create a SitePage for each Site is if you anticipated wanting to have a lot of custom CMS content for different SitePages. However, I do not think that is necessary here. I imagined each Site Page would be dynamically generated based on a set of info common to each Site (e.g. name, URL, scan results).

@GabeIsman
Copy link
Contributor Author

Totally agree that best practices are not obvious.

I didn't know about ModelAdmin, that seems really useful. It almost perfectly fits this use case, however I think there are a couple of other considerations.

  1. With the SitePage approach we get a nice inline selector menu on other pages to choose a site. This is relevant for the homepage, where we want to display stats on some subset of sites. This was actually the original reason I chose this approach.
  2. I think it's totally possible that there would be CMS-controlled content on a site page. For example, I think somebody mentioned at one point putting in a link to the article/post announcing a site's intention to implement HTTPS. Possibly short success stories or congratulatory notes for sites that made the transition. Even if these things are not in the immediate plan it seems plausible that they might come up.

Also as a side note, I think that we should definitely avoid requiring admins to know about django-admin. Two admin interfaces is bound to be super confusing.

@GabeIsman
Copy link
Contributor Author

GabeIsman commented Aug 25, 2016

Actually looking more into wagtail/wagtail#231 we may need to implement a custom workaround for 1. anyway. Need to look into it a bit more.

@GabeIsman
Copy link
Contributor Author

This is superseded by #4.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants