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

Allow footer text to derive from meta.json #707

Closed
trvrb opened this issue Feb 26, 2019 · 32 comments · Fixed by #834
Closed

Allow footer text to derive from meta.json #707

trvrb opened this issue Feb 26, 2019 · 32 comments · Fixed by #834
Assignees
Labels
enhancement New feature or request

Comments

@trvrb
Copy link
Member

trvrb commented Feb 26, 2019

Currently text in footer (https://github.com/nextstrain/auspice/blob/master/src/components/framework/footer.js#L49) is the only remaining place where auspice source code has to get modified in a dataset specific manner. Notably, this currently causes issues with community builds, where for example Paul's work here: https://nextstrain.org/community/pauloluniyi/lassa/s, gets a footer borrowed from here: https://nextstrain.org/lassa/s, which is inaccurate and doesn't appropriately highlight Paul's work.

I recommend adding a footer field to meta.json that auspice uses to specify this text. Probably best to have this be HTML and so people can embed links. If footer is present auspice uses this. If not auspice falls back on current strategy of window.location.pathname.includes.

@trvrb trvrb added the enhancement New feature or request label Feb 26, 2019
@tsibley
Copy link
Member

tsibley commented Feb 26, 2019

HTML might be ok in the short-term to get this going now, but pretty soon we will need to address the security implications of allowing that HTML injection. It might be easiest to not allow it from the beginning, particularly since nextstrain.org already allows rendering of untrusted inputs via community URLs.

We might address the security issues by filtering to a minimal allowed set of tags or by switching to some other, more restricted format which we render to safe HTML.

@trvrb
Copy link
Member Author

trvrb commented Feb 26, 2019

Ah! Good point @tsibley. Would markdown be a safe alternative?

@tsibley
Copy link
Member

tsibley commented Feb 26, 2019

Yes, if whatever we use to render Markdown → HTML has the appropriate restrictions, since most Markdown parsers allow inlining raw HTML and some allow arbitrary element attributes.

@jameshadfield
Copy link
Member

Currently the auspice server (and nextstrain.org server) include a node markdown library which is used to parse the narrative markdown files, sending (sanitized) HTML to the client to render. With the proposed server API, the parsing of the footer markdown (and also the narratives) would need to happen on the client.

@jameshadfield
Copy link
Member

I think best to leave this until after v2 release. We can add in a footer field in the v2 JSONs at a later date and maintain backwards compatibility.

@jameshadfield
Copy link
Member

Update: This "feature" is really important as we (and others) start to integrate auspice into other projects. The dataset JSON should be able to define this. In parallel, this should be made part of the augur auspice-export JSON, and the footers currently defined in auspice moved into their respective augur builds.

Note that a few footers contain "links" which dispatch redux actions (e.g. https://github.com/nextstrain/auspice/blob/master/src/components/framework/footer.js#L179). This functionality can be achieved by specifying it as a link to the same dataset with the view parameters encoded in the URL. Not quite as smooth, but good enough in my opinion.

@joverlee521
Copy link
Contributor

@jameshadfield I'm slightly unclear on when the footer text is added to meta.json.

Are we going to provide the option within augur export to add the footer text or will users have to manually add the footer text to meta.json?

Also, some of the hardcoded text is quite long and makes me question if we want to have it within meta.json. Can we have a separate Markdown file alongside meta.json that contains the footer text?

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

My preference would be to publish a Markdown file alongside the JSONs as another sidecar file (e.g. zika_acknowledgements.md) which Auspice tries to fetch and, if successful, renders. This avoids the need to change augur export or update JSONs when tweaking acknowledgements.

I think any new option to augur export to add acknowledgements to the meta.json would have to read from a Markdown file anyway, so easier to just publish the file directly.

@jameshadfield
Copy link
Member

jameshadfield commented Dec 10, 2019

I can understand the simplicity of creating a separate markdown file which auspice fetches (akin to how the current tip-frequencies JSON is currently fetched, and how the root-sequence will be fetched). I'm happy to have it done this way.

Also, a note regarding meta.json -- the concept of a dedicated meta.json is being replaced with the concept of a "unified" JSON (i.e. the main dataset JSON exported by augur export v2), which contains a top-level key meta. If we decide against using a specific markdown file, then I don't think it's worth including it in the meta.json from augur export v1, but rather focus on including it in the main json produced by augur export v2.

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

the concept of a dedicated meta.json is being replaced with the concept of a "unified" JSON

Yep, I know, I was just using meta.json as a lingering shorthand. :-)

@joverlee521
Copy link
Contributor

Even if we use a separate markdown file for the footer text, the dataset JSON should probably still have a meta.footer key that points to the specific markdown filename so we don't run into the same string matching problem as window.location.pathname.includes

@jameshadfield
Copy link
Member

jameshadfield commented Dec 10, 2019

Even if we use a separate markdown file for the footer text, the dataset JSON should probably still have a meta.footer key that points to the specific markdown filename so we don't run into the same string matching problem as window.location.pathname.includes

We've discussed this previously in a similar context, namely when should auspice fetch a certain sidecar JSON. The decision we made was that, in general, this should not be encoded in the dataset JSON but that auspice should always make the requests and be OK with them 404ing. In the current auspice request API -- https://nextstrain.github.io/auspice/server/api#charon-getdataset -- this would be getDataset?prefix=X&type=acknowledgements and then the nextstrain.org server would fetch a specific file name, e.g. X_acknowledgements.md. I think best to implement this issue in a similar way to our current design ideas & implementations.

@trvrb
Copy link
Member Author

trvrb commented Dec 10, 2019

Yes. I think it makes sense to have a separate "sidecar" file _footer.md for this that has strict naming enforced (ala _tip-frequencies.json and _root-sequence.json). In terms of where this file "lives":

If this _footer.md file lives in auspice/ it's going to get removed by snakemake clean (I wipe things all the time this way when restarting a build). It either needs to live in config/ and get moved by the build or augur export needs to move it to auspice/. An advantage to packaging this with augur export would be that augur export could ensure file naming is appropriately matched to the main dataset.json.

So, my suggestion is actually to add a --footer option to augur export that takes in a .md file and renames it appropriately alongside the main dataset export. I say footer rather than acknowledgements just to cut down on verbosity.

Note that a few footers contain "links" which dispatch redux actions (e.g. https://github.com/nextstrain/auspice/blob/master/src/components/framework/footer.js#L179). This functionality can be achieved by specifying it as a link to the same dataset with the view parameters encoded in the URL. Not quite as smooth, but good enough in my opinion.

I agree with this solution.

@trvrb
Copy link
Member Author

trvrb commented Dec 10, 2019

I realize now that packaging with 'augur export' will be pretty necessary. For cases like seasonal flu we have 32 datasets produced that share the same footer md. In this scenario, we'd have the same config/footer.md that is passed to 'augur export' for each of the 32 datasets.

It's a little weird to have 32 copies of the same file with different names pushed up to S3, but other solutions (like specifying file path in dataset.json) add significant complexity.

@emmahodcroft
Copy link
Member

While it's perhaps a little weird to have 32 copies of the file for the flu builds, I think this might be a bit of an outlier scenario?
I think for most datasets/uses this isn't going to be the case, and then the 'fetch-if-possible-based-on-strict-naming' approach seems most straightforward.

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

I agree with @emmahodcroft, and while 32 copies of the file is a little weird maybe, it doesn't pose any material problems.

I say footer rather than acknowledgements just to cut down on verbosity.

I don't want to bikeshed this, so will give my 2¢ here and then shut up: "Acknowledgements" is more specific and descriptive than "footer" and is a term everyone will recognize. "Footer" is tied to page layout and is less communicative about what's intended to go in there. It raises questions like "Which footer is it?" (there are several ostensible footers in Auspice) and "What happens to the filename if Auspice starts putting acknowledgments elsewhere on the page?" (which it already does in the "Download data" modal). Verbosity is not a great reason for choosing a less precise term.

@trvrb
Copy link
Member Author

trvrb commented Dec 10, 2019

This makes complete sense @tsibley. I see the rationale for choosing a semantically meaningful term over a layout based term. However, I would suggest in this case something more like _description.md rather than _acknowledgements.md. There's already examples where the "footer" text is used for more than just acknowledgments and I could see this being popular. What do you think?

wnv

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

Sure, I think _description.md would be fine. Another option might be _readme.md, although that is more obscure a term than "description".

@joverlee521
Copy link
Contributor

After looking more at the charon API, using a markdown file might be more trouble than it's worth. Everything expects/returns JSON, so there would have to be conditionals littered throughout the API structure to ensure that the correct content type is returned.

@trvrb
Copy link
Member Author

trvrb commented Dec 10, 2019

I'll let @jameshadfield / @tsibley speak to the technical working of charon. On the user side, I would be fully okay with having augur export called with --description config/description.md and have augur export produce a _description.json file that is a light wrapping of the Markdown. Non-ideal, but not terrible. To prevent HTML injection we'd still want to import Markdown into auspice rather than HTML.

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

Yep, that's what @joverlee521 and I chatted about a few minutes ago, with the difference of putting the Markdown string into the main JSON directly instead of putting it a sidecar. Since it's going into JSON now, there seems little reason to keep it a sidecar.

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

To prevent HTML injection we'd still want to import Markdown into auspice rather than HTML.

To be clear on this though, the HTML produced by the rendering of the Markdown still needs to be sanitized.

@trvrb
Copy link
Member Author

trvrb commented Dec 10, 2019

I was going to suggest going with a single file but I was worried about augur export --minify-json messing with the whitespace that Markdown requires.

@tsibley
Copy link
Member

tsibley commented Dec 10, 2019

Ah, that minification only affects the JSON structure itself, not any values in the JSON, which are always serialized as-is.

@joverlee521
Copy link
Contributor

Two questions:

  1. How important is it to keep the ability to open links in a new tab?
  2. Do we want to allow images? (there are currently only images in the seasonal flu builds)

@tsibley
Copy link
Member

tsibley commented Dec 11, 2019

  1. Doesn't seem important to me, but others may have opinions.
  2. Sure, though worth being aware this will let dataset authors track page views. (Which is probably just fine.)

@trvrb
Copy link
Member Author

trvrb commented Dec 11, 2019

  1. I think moderately important. Would be nice if links opened in new tab, but not critical.
  2. Allowing images seems also like a good thing to aim for. Would mainly exist to give a place for people to drop logos, etc... (But this could also argue for a separate logos array to ensure proper sizing and alignment, don't know what's best here)

@joverlee521
Copy link
Contributor

  1. I believe the link would have to be in raw HTML within the Markdown to allow links to open in new tabs.
  2. Also unsure about how styling can be done without raw HTML within Markdown.

@trvrb
Copy link
Member Author

trvrb commented Dec 11, 2019

I had this same issue on bedford.io. I'll look up what I did.

@tsibley
Copy link
Member

tsibley commented Dec 11, 2019

  1. Generally how this works is that there's an HTML-rewriting phase after the Markdown → HTML → sanitized HTML process that can add target=_blank rel="noreferrer nofollow" and other attributes.

  2. Requiring HTML in the Markdown for image alignment seems like a fine trade-off?

@trvrb
Copy link
Member Author

trvrb commented Dec 12, 2019

In case it's helpful. Here's what I did for bedford.io, where I wanted Markdown image links in blog posts to be properly centered. See for example images here: https://bedford.io/blog/avian-influenza-h7n9/, specified in this Markdown: https://raw.githubusercontent.com/blab/blotter/master/blog/_posts/2017-03-10-avian-influenza-h7n9.md

The entire post div that wraps the Markdown content gets a post class: https://github.com/blab/blotter/blob/master/_layouts/post.html#L71

Then img with .post get CSS: https://github.com/blab/blotter/blob/master/css/style.less#L230

I'd think it to be most common to want a single logo-type image centered in the footer, and I might default to centering (which could then be overridden by HTML within the Markdown post itself).

@tsibley
Copy link
Member

tsibley commented Dec 12, 2019

Defaulting to centering of images for the dataset description/footer makes sense to me. If we use flexbox, it could even work for multiple images (like seasonal flu is already doing), as long as the <img>s are in a single identifiable element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants