Opened 2 years ago

Closed 19 months ago

#23854 closed enhancement (fixed)

Add an RSS feed for https://metrics.torproject.org/news.html

Reported by: cypherpunks Owned by: irl
Priority: Medium Milestone:
Component: Metrics/Website Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points: 2.5
Parent ID: Points:
Reviewer: Sponsor:

Description

This is an item on the roadmap and has also been requested on twitter:
https://twitter.com/33b5e5/status/918598903066386432

Child Tickets

Attachments (1)

UpdateNews.java (4.3 KB) - added by karsten 19 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 2 years ago by karsten

Yes, this is something we'd like to provide.

Couple of questions regarding a possible implementation:

  • What formats and versions should we provide? I'm not using RSS/Atom feeds myself, so I can't say what's most useful. Nor can I say what formats one would provide when starting to provide a feed today. So, would we want RSS or Atom or both or something else? And which version or versions would we want to provide?
  • Are there specific parts of news that we should include, and are there ways to provide data in ways that are easier (or unnecessarily harder) to process for feed clients?
  • Are there (implicit) conventions regarding feed URLs? For example, should we provide the RSS feed for https://metrics.torproject.org/news.html under something like https://metrics.torproject.org/news.rss, or are there no such conventions?
  • What do people usually use as library for creating feeds in Java? A quick search reveals ROME as possible candidate: "ROME is a Java framework for RSS and Atom feeds. It's open source and licensed under the Apache 2.0 license." But I have no idea if I'm overlooking an obviously better library.

Thanks for any input on these questions. Any (useful) answers on these will speed up the implementation process.

comment:2 Changed 21 months ago by irl

Cc: irl added

comment:3 Changed 19 months ago by irl

Owner: changed from metrics-team to irl
Status: newaccepted

Moving into my queue.

comment:4 Changed 19 months ago by irl

Done a little hacking on this, I think my plan looks like:

  • Add a formatAsFeedItem method to org.torproject.metrics.web.News
  • Point the resource /feed.xml at NewsServlet
  • Add a JSP named feed.jsp, which is used instead of news.jsp to render the feed version
  • Implement only ATOM (RFC4287), not RSS, as ATOM has far better support for internationalisation that may be used later, where internationalising RSS may hold back that work

This would be the way to do it to match the way the news page is currently built. I do wonder though if perhaps we should instead refactor the way the news page works:

  • Remove the formatAsTableRow method from org.torproject.metrics.web.News
  • Make getters for org.torproject.metrics.news.News public
  • Re-implement formatAsTableRow using JSTL in news.jsp
  • Point the resource /feed.xml at NewsServlet
  • Add a feed.jsp using JSTL

We definitely do not need to add ROME, or a similar library, at least for now. As an example to get an idea, the RSS 0.91 hacky JSP (where I made getDescription public so that it could be available):

<%@ page contentType="text/xml" %><?xml version="1.0"?>
<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>
<%@ taglib prefix="fn" uri="http://java.sun.com/jsp/jstl/functions" %>
<rss version="0.91">
<channel>
    <title>Tor Metrics - News</title>
    <link>https://metrics.torproject.org/</link>
    <c:forEach var="entry" items="${news}">
      <item>
        <title>
          <c:out value="${entry.description}"/>
        </title>
      </item>
    </c:forEach>
</channel>

karsten, iwakeh - Which approach do you think would be best?

comment:5 Changed 19 months ago by irl

Cc: metrics-team added; irl removed

karsten, iwakeh - Please see comment:4, I forgot to add metrics-team back to CC when taking the ticket.

comment:6 in reply to:  4 Changed 19 months ago by karsten

Replying to irl:

Done a little hacking on this, I think my plan looks like:

  • Add a formatAsFeedItem method to org.torproject.metrics.web.News
  • Point the resource /feed.xml at NewsServlet
  • Add a JSP named feed.jsp, which is used instead of news.jsp to render the feed version
  • Implement only ATOM (RFC4287), not RSS, as ATOM has far better support for internationalisation that may be used later, where internationalising RSS may hold back that work

Sounds good!

This would be the way to do it to match the way the news page is currently built. I do wonder though if perhaps we should instead refactor the way the news page works:

  • Remove the formatAsTableRow method from org.torproject.metrics.web.News
  • Make getters for org.torproject.metrics.news.News public
  • Re-implement formatAsTableRow using JSTL in news.jsp
  • Point the resource /feed.xml at NewsServlet
  • Add a feed.jsp using JSTL

Hmm, I'm not sure how complex that JSTL variant of formatAsTableRow would become. Also keep in mind that we're using formatted news items in NewsServlet and GraphServlet.

We definitely do not need to add ROME, or a similar library, at least for now. As an example to get an idea, the RSS 0.91 hacky JSP (where I made getDescription public so that it could be available):

<%@ page contentType="text/xml" %><?xml version="1.0"?>
<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>
<%@ taglib prefix="fn" uri="http://java.sun.com/jsp/jstl/functions" %>
<rss version="0.91">
<channel>
    <title>Tor Metrics - News</title>
    <link>https://metrics.torproject.org/</link>
    <c:forEach var="entry" items="${news}">
      <item>
        <title>
          <c:out value="${entry.description}"/>
        </title>
      </item>
    </c:forEach>
</channel>

Great that we don't need to add a library.

karsten, iwakeh - Which approach do you think would be best?

I'd say let's start with the first plan. Thanks for researching these alternatives!

comment:7 Changed 19 months ago by irl

Ah ok, I hadn't seen it used in GraphServlet also. Will go for the first plan.

comment:8 Changed 19 months ago by irl

I have implemented the first plan in my task/23854 branch. There are a couple of special cases that I'd like to not have to deal with though.

Currently the news.json assumes that the output will be HTML and that it's OK to have HTML inline, but ATOM is XML and has a different root namespace.

Can you point me at the tool you're using to import changes from the wiki? I wonder if I can do a little sanitising of the data there that would greatly simplify the output of the ATOM and remove the special cases.

Changed 19 months ago by karsten

Attachment: UpdateNews.java added

comment:9 in reply to:  8 Changed 19 months ago by karsten

Replying to irl:

Can you point me at the tool you're using to import changes from the wiki? I wonder if I can do a little sanitising of the data there that would greatly simplify the output of the ATOM and remove the special cases.

Please find it attached. I'm using jq "." news-raw.json > news.json to produce the final output.

If you feel like cleaning up this code a bit and adding it to the tree, great. But feel free to send me a modified version back, and we'll add it to the repository when we have more time.

comment:10 Changed 19 months ago by irl

The first approach is making me write code that makes me very sad. It hides presentation steps as data pre-processing steps.

I'd like to take the second approach instead, with the reusable portion (each row of the table) inserted as a include in the JSPs for GraphServlet and NewsServlet.

This has benefits later for localisation, for the styleguide updates, and it would be really easy to add an iCal feed in addition to the RSS/ATOM.

I'd also like to change the format of news.json to not presume that output is going to only be HTML. Does anything else consume news.json other than GraphServlet and NewsServlet?

comment:11 Changed 19 months ago by karsten

Alright, please go ahead. (Nothing else consumes news.json than the two classes you mention.)

comment:12 Changed 19 months ago by irl

Cool. (:

comment:13 Changed 19 months ago by irl

Status: acceptedneeds_review

Please review my branch task/23854. It has been rebased onto master just now, and the feed tested in Firefox/Thunderbird. Don't forget to make sure I haven't broken NewsServlet or GraphServlet's existing functionality (I don't think I have).

Note that for the JSTL rendering the table rows in the news and graph JSPs, this is a static include to have it happen at translation time.

comment:14 Changed 19 months ago by karsten

Status: needs_reviewneeds_revision

Alright, I did a first review by reading commits without trying them out yet. Here's what I found:

  • You should read about "Java diamond operator". In several places you're undoing changes that we made quite recently when switching to Java 7. For example, we prefer List<String> aList = new ArrayList<>(); over `List<String> aList = new ArrayList<String>();'.
  • Please add a space after the closing bracket in return (String[]) this.protocols.toArray() and related places.
  • I think your code prints a news entry with start == end as "start to end", whereas the current code would print it as just "start". Can you change that back?
  • You picked non-standard Java variable names like short_desc and fixed them in a later commit. Can you rather make such fixes to your own commits as "fixup" or "squash" commits and either squash them yourself or let me do that as part of the merge process?

comment:15 in reply to:  14 Changed 19 months ago by irl

Status: needs_revisionneeds_review

Replying to karsten:

  • You should read about "Java diamond operator". In several places you're undoing changes that we made quite recently when switching to Java 7. For example, we prefer List<String> aList = new ArrayList<>(); over `List<String> aList = new ArrayList<String>();'.

Fixed.

That's going to be a hard habit to break. Is there something we can add to checkstyle to catch this?

  • Please add a space after the closing bracket in return (String[]) this.protocols.toArray() and related places.

Fixed.

  • I think your code prints a news entry with start == end as "start to end", whereas the current code would print it as just "start". Can you change that back?

Fixed.

  • You picked non-standard Java variable names like short_desc and fixed them in a later commit. Can you rather make such fixes to your own commits as "fixup" or "squash" commits and either squash them yourself or let me do that as part of the merge process?

Fixed.

comment:16 Changed 19 months ago by irl

Actual Points: 2.5

comment:17 Changed 19 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Looks good! Merged and deployed! Yay. :)

Regarding checkstyle and the diamond operator, I don't know whether we can put in a check specifically for that.

Closing. Thanks!

Note: See TracTickets for help on using tickets.