-
Notifications
You must be signed in to change notification settings - Fork 15
RSS configuration and a simple admin panel #96
Conversation
This enables us to get around any CORS issues with the podcast RSS feed. We just re-serve it from our server with CORS already enabled. By parsing the RSS serverside and serving it up as plain json, we no longer have to include a costly XML parsing step on the client. #80
There is now the option to point shortcut's client and server to an RSS feed URL instead of maintaining an `episodes.json` file with manual metadata. The admin sets the `useRSS` option in client/src/config/default.js to `true` and provides a feed URL in the server's .env file `RSS_FEED`. At every point in the code where `episodes.json` might be accessed for metadata about all episodes, the code now branches if RSS is enabled and instead parses the RSS feed. This closes #82.
There is now an admin panel that can be reached at the `/#/admin` route. The panel shows a list of episodes that Shortcut knows about, as well as their state as enabled or disabled (disabled by default). This state is stored in `server/adminData.json` on the server. This closes #81, because we no longer need a specific user-defined GUID to say "episode 3098281ABD23 is enabled" -- now we use the `guid` tag defined in the rss spec.
Fixes the following warning reported: npm WARN [email protected] No repository field.
Whilst Yeoman may have been used during initial, one-off generation of the project it is unlikelt to be re-run. Remove the configuration file from upstream repository. Unlikely to be run again as: (1) yeoman itself isn't a dependency of this project, and (2) low likelihood of re-running a project generator after v1.0.0 has been released without a major refactor.
Use new abstracted name in client/ documentation.
Update documentation and example commands in server/ to reflect new project name.
Documentation and package.json provide guidance to using nodemon to ease the development process. However, it was missing from devDependencies in server/package.json and a manual install instruction in server/README.md As npm has functionality to reflect dependencies that should not be run in --production (or when the NODE_ENV environment variable is set to production) then let's utilize that. This reduces the number of manual commands that must be run to setup the recommended shortcut development environment.
Rename the existing test to *Test.js, so that shortcut-client's test runner automatically finds the existing test for ClippingContainerComponent. This improves code coverage as measured by lines to 25.57% (from 23.56%). See below: BEFORE --------------------------------|----------|----------|----------|----------|----------------| File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines | --------------------------------|----------|----------|----------|----------|----------------| ... src/components/ | 45.99 | 34.85 | 65.77 | 27.17 | | ClippingContainerComponent.js | 45.96 | 25.62 | 74.19 | 22.43 |... 441,442,444 | ... --------------------------------|----------|----------|----------|----------|----------------| All files | 42.71 | 35.54 | 62.18 | 23.56 | | --------------------------------|----------|----------|----------|----------|----------------| AFTER --------------------------------|----------|----------|----------|----------|----------------| File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines | --------------------------------|----------|----------|----------|----------|----------------| ... src/components/ | 48 | 37.02 | 66.15 | 29.78 | | ClippingContainerComponent.js | 63.35 | 41.32 | 77.42 | 45.79 |... 441,442,444 | ... --------------------------------|----------|----------|----------|----------|----------------| All files | 44.25 | 37.16 | 62.95 | 25.57 | | --------------------------------|----------|----------|----------|----------|----------------|
Without this peer dependency of [email protected], the following error message is reported when running `npm install`: [email protected] /home/rhyskidd/Coding/shortcut/client └── UNMET PEER DEPENDENCY flexboxgrid@^6.3.0 npm WARN [email protected] requires a peer of flexboxgrid@^6.3.0 but none was installed. Reported with: $ npm --version 3.5.2 $ nodejs --version v6.11.4 For more on peer dependencies, see: https://stackoverflow.com/a/22004559/4206728
If no RSS_FEED is specified on the server, the `rss/` endpoint immediately returns a 404 instead of attempting a GET request. Relevant to #95.
Fixes a bug where two cache objects were created; we now create a single cache object on server init, and then pass it into `routes/all-episode-data.js`. Forcing the entire cache to update every time, since this is only ever called during occasional admin-only tasks. (all `cache.save()` calls are now passed `true`); Also updates the Admin panel to grab the full list of episodes completely unfiltered from the server (via a call to `/recent?filter=0`). Before this commit, disabling an episode from appearing on the front page would also disable it from appearing in the admin panel! (The unfiltered endpoint doesn't need to be secure since it's pulling from the canonical rss anyway.)
This commit makes the admin panel control what appears on the front page even if Shortcut is set up to use `episodes.json` as its metadata source instead of RSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, @dariusk!
Left some comments / questions, mostly minor things
client/src/components/Main.js
Outdated
if (env === 'dev') { | ||
let devUrl = useRSS ? apiEndpoint + '/rss' : apiEndpoint + '/recent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the /rss and /recent endpoints return the same result. I'm probably missing something, but why do we need to be able to access both from the client (and are we only hitting the /rss
endpoint in dev)? It'd be nice if we could get rid of this useRSS
flag on the client so that only the server needs to know the datasource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I had originally envisioned the /rss
endpoint to be a simple local mirror of the RSS feed to get around CORS issues, but I think it makes sense to just keep /recent
and have it render the rss data if that's enabled on the server. And then no useRSS
flag!
Re: only hitting that endpoint in dev, that's technically true but what is really happening is in production, we render the front page using the marko template, which loads the episodes using the allEpisodeData
function, which in turn checks for RSS and preloads those episodes, or falls back to the old explicit json method.
"striptags": "^2.1.1", | ||
"twit": "^2.2.9", | ||
"urlsafe-base64": "^1.0.0", | ||
"waveform-util": "github:howlingeverett/waveform-util" | ||
"waveform-util": "github:FeelTrainCoop/waveform-util" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
server/routes/all-episode-data.js
Outdated
request.get({url: rssFeed}, function(err, resp, body) { | ||
if (!err) { | ||
let episodes = cache.getKey('episodes') || []; | ||
console.log('AAAA',episodes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this console.log (and the leftover one above on line 17 while at it, I think I'm to blame for that one 🙃 )
client/src/components/Admin.js
Outdated
</div> | ||
</div> | ||
<div className="content episodes"> | ||
<h3 className="recent-episodes">Enable/Disable Episodes</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users can see this before authenticating. Maybe that's ok cuz why would they be on this page in the first place? But we could wait until the withCredentials
request above succeeds, then set component state to authenticated
and render the text that says "Admin Panel" etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing exactly this, good call.
enabled: switches[index].checked | ||
} | ||
}); | ||
this.forceUpdate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we do this without forceUpdate
? Maybe use the POST response to update the component state? I've read not to use forceUpdate, but probably not a big deal here and I do like how simple this component is otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I was unable to figure out the React lifecycle stuff to make an update happen in a way that made sense here without just doing a forceUpdate
. I'm okay with it for now because it's admin-facing, but generally I wouldn't do it if it were a listener-facing feature.
client/src/components/Admin.js
Outdated
return episode; | ||
}); | ||
this.setState({ | ||
switches: tempSwitches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe set "authenticated" here?
server/README.md
Outdated
- `docker build -t tal-server .` | ||
- `docker run -p 3000:3000 -d tal-server` | ||
- `docker build -t shortcut-server .` | ||
- `docker run -p 3000:3000 -d shortcut-server` | ||
- server will be running at docker-machine ip, i.e. `http://192.168.99.100:3000/` | ||
|
||
### Run with Node: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of the es6 node requires 6.x or higher? It says 4.4.3 down here so we should update and maybe add engines to package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted the line in question to avoid confusion
server/package.json
Outdated
"author": { | ||
"name": "Darius Kazemi", | ||
"email": "[email protected]" | ||
}, | ||
"license": "GPL-3.0", | ||
"devDependencies": { | ||
"nodemon": "^1.10.0" | ||
}, | ||
"dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add { "engines" : { "node" : ">=6" } }
here?
server/routes/all-episode-data.js
Outdated
// filter out inactive episodes specified in the .env file | ||
.filter((episode) => !inactiveEpisodes.includes(episode.number)) | ||
// filter out inactive episodes (only include explicitly active episodes in the admin panel) | ||
// unless we passed `true` to `episodes` in which case, include all episodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when do we pass true
instead of an array of episodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff was left over from an earlier instance of the code, before I gave the option to return filtered or unfiltered episodes. Fixing now!
server/routes/admin.js
Outdated
// return the state of enabled/disabled episodes | ||
// any episode not in this list is considered disabled by default | ||
router.get('/getEpisodes', function (req, res) { | ||
return res.json(req.app.get('cache').getKey('episodes')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to check the RSS feed for new episodes here?
Otherwise, I'm not totally clear on is when/how we check the RSS feed for new episodes—it seems this only happens when we boo the server or if we're in development mode and hit /rss
on the Landing page from the frontend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, good point. As it stands the workflow would be: publish new episode, get your transcript in order, reboot the server, enable in admin panel. Probably shouldn't have to reboot the server every time. Maybe a button on the admin pane that says "refresh all episodes" which updates the episode cache from the live RSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, better idea, and closer to what you suggested: just update the cache itself here, since it's an admin pane. Just need to make sure it happens after authentication.
* Explicitly declare Node.js compatibility in the package.json * Replace a console.log with a thrown Error object * Safety-check the parent object of a subobject before testing if the subobject exists From @therewasaguy's PR feedback.
Infinite scroll is overengineering for this particular UX element. Even the weakest smartphones can render ~1000 plaintext divs no problem, and while getting all the way down to the bottom might be a problem, as long as we don't provide a search feature (which is forthcoming) at least rendering all the episodes on one landing page lets a user ctrl+F or "find in page" on mobile to find the particular episode they are looking for. Even pagination (the usual alternative to infinite scroll) would make this impossible.
This commit makes it so we render a blank page on the Admin page until the user successfully authenticates. Previously we rendered an empty Admin panel but probably best to obfuscate all Admin functions prior rendering instead of giving an empty placeholder admin panel. (Per @therewasaguy's suggestion)
Per @therewasaguy's [review](#96 (comment)) the '/rss' and '/recent' endpoints return the same thing if you're set to RSS mode. So no reason to have a dedicated '/rss' endpoint, and also no more need for a `useRSS` configuration variable on the client. The client just asks the server for recent episodes, and the server knows if it needs to retrieve those from the RSS feed or from the JSON file.
Cleaned up some jshint warnings and updated moment which was throwing a bad warning.
The all-episode-data route used to have some gnarly logic built in but I removed that a while back. However, some confusing comments stayed in, and also some application logic that we didn't need was in there too. This commit removes those.
Accessing the admin panel will update the cache locally. It's just a GET request to the RSS feed (in the case of RSS), which we wouldn't want a normal user to intialize but is fine if an admin does it. The server endpoint is behind an auth gate so only an admin will ever kick it off.
Major new feature which closes epic #76, including sub-issues #80, #81, #82, #90, #95
RSS_FEED
server.env
variable. If this points to a valid RSS feed, it returns a JSON-parsed version of all the feed data if a GET request is made to http://localhost:3000/rss (in dev, replace with the server name as needed)useRss
variable inclient/src/config/base.js
which tells the client to use the RSS data instead of accessingepisodes.json
for metadataepisodes.json
or the RSS feed. An administrator can provide credentials to access this panel and then enable/disable episodes from appearing on the front page as needed. Episodes are considered disabled by default. 86d2f8fADMIN_PASSWORD
server.env
variable, which is the password for simple http auth for the admin panel. 0fddb27guid
(string) andenabled
(bool) in the form body to enable/disable episodes by GUIDepisodes.json
config, the guid is simply the episode number, since no two episodes will presumably share an episode number. However, if RSS configuration is enabled, it will use theitem:guid
tag from the RSS feed itself as the guid, and if no guid is specified it uses the canonical uri for the itemitunes:episode
anditunes:season
tags in the RSS, or it numbers them on its own in chronological order by publication date.adminData.json
at the server root. 86d2f8fDocumentation follows. This will replace the Create the episode metadata section in the Importing Your Own Podcast wiki article.
Provide the episode metadata
There are two ways to give Shortcut the metadata for your episodes:
episodes.json
file (what the sample project does)We highly recommend using the RSS method since it's one less manual step for you as Shortcut will automatically derive episode titles, publish dates, etc from the RSS. If you don't publish your full RSS history, please consider doing so! Even hundreds of episodes will come out to a sub-1MB file. However, we understand that some podcasts just can't do this, so we also provide the manual JSON method for those shows.
Option 1: Manually specify episode data
Open a text editor to an empty file and enter the following JSON for your test episode, with the appropriate metadata included:
Save this in the Shortcut data directory as
episodes.json
. Now your directory should look like:Option 2: Point the server to your full RSS feed
In your
server/.env
file, setRSS_FEED
to the URL of your full RSS feed, like so:Note: if you have this set, your server will now locally serve your RSS feed from http://localhost:3000/rss. This endpoint returns a 404 if no feed (or a bad feed) is specified.
Enable your test episode
If you browse to http://localhost:8000/#/admin you will see an administration panel with every episode of your podcast.
You will be asked for a username and password. The username is
admin
, the default password is1234
. Use this for now while testing, then immediately change the password to something else by changingADMIN_PASSWORD
in yourserver/.env
file. Please generate a secure password!In the case of an RSS configuration you will see literally all the episodes; in the case of the
episodes.json
configuration you will see just the episodes you specified in that file.Each episode has an on/off switch which should be set to "off" by default. Switch this to "on" for your test episode, then go back to http://localhost:8000 and reload the page if necessary to see the episode you just enabled listed on the home page.
At this point, you can click the episode but it will return an error because we have a couple more steps to do.