-
Notifications
You must be signed in to change notification settings - Fork 94
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
Podcast RSS final touches #317
Conversation
@@ -375,7 +375,7 @@ to consume our videos: an RSS feed that can be used with podcast apps! | |||
], | |||
[ | |||
text( | |||
String(url(to: .account(.rss(userId: user.id, rssSalt: user.rssSalt))).prefix(45)) | |||
String(url(to: .account(.rss(userId: user.id, rssSalt: user.rssSalt))).prefix(40)) |
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.
trimming this a bit cause the screenshot test showed it didn't fit on smaller devices.
@@ -269,6 +269,8 @@ please contact support@pointfree.co. | |||
|
|||
func clearHeadBody<I>(_ conn: Conn<I, Data>) -> IO<Conn<I, Data>> { | |||
return IO { | |||
// TODO: this doesn't actually work. The `conn.request.httpBody` has all the | |||
// data, and that's what needs to be cleared. |
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 I tried writing a test for the HEAD request I realized this doesnt do what I expected. we need to be able to clear the request's body, which means this helper needs to live in HttpPipeline.
@@ -8,7 +8,7 @@ let atomFeedResponse = | |||
writeStatus(.ok) | |||
>=> respond(pointFreeFeed, contentType: .application(.atom)) | |||
|
|||
let episodesRssMiddleware: Middleware<StatusLineOpen, ResponseEnded, SubscriberState, Data> = | |||
let episodesRssMiddleware: Middleware<StatusLineOpen, ResponseEnded, Prelude.Unit, Data> = |
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 dont know why this middleware took SubscriberState
since it's an unauthenticated endpoint. luckily I wasn't actually using it anywhere, so I was able to remove it easily from a few spots.
? """ | ||
Subscriber-Only: Today's episode is available only to subscribers. If you are a Point-Free subscriber you \ | ||
can access your private podcast feed by visiting \(url(to: .account(.index))). | ||
switch episode.permission { |
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.
here's where I provide 3 different types of copy for the episode description / show notes based on the episode's permission.
Sources/PointFree/AtomFeed.swift
Outdated
@@ -153,7 +167,7 @@ that day! | |||
description: description(episode: episode), | |||
dublinCore: .init(creators: ["Brandon Williams", "Stephen Celis"]), | |||
enclosure: enclosure(episode: episode), | |||
guid: url(to: .episode(.left(episode.slug))), | |||
guid: String(describing: episode.freeSince ?? episode.publishedAt), |
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.
using the episodes free date / published date as the GUID allows us to create a new entry for episodes that were subscriber only and then switched to free.
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.
Is String(describing:)
subject to weird local/locale formatting? Should we use a DateFormatter
to be safe for testing?
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.
oh im not sure. I could also just use the underlying time interval. how's that sound?
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.
Oh yeah that sounds perf!
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.
coo done 5fd134f
|> respond( | ||
body: view.rendered( | ||
with: conn.data, | ||
config: Current.envVars.appEnv == .testing ? .pretty : .compact |
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.
pretty printing for our atom feed tests
case production | ||
case staging | ||
case testing |
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.
added a testing
case so that we could pretty print only for tests. we could prob also get rid of staging.
exercises: [Exercise], | ||
fullVideo: Video? = nil, | ||
id: Id, |
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.
somehow this got out of order, had to fix all the episodes too
$0.isEnabled | ||
|| ($0.isAdminEnabled && user?.isAdmin == .some(true)) | ||
} | ||
?? false |
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.
ok, the whole [Feature].hasAccess(to:)
API is so whack. I dont know what I was thinking when I wrote it. here I just did a minimal amount of work to make it sensible, but we should revamp it sometime soon.
@@ -121,17 +121,31 @@ func respond<A, B>( | |||
>=> respond( | |||
body: pageLayout.rendered( | |||
with: newLayoutData, | |||
config: Current.envVars.appEnv == .production ? .compact : .pretty | |||
config: Current.envVars.appEnv == .testing ? .pretty : .compact |
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.
let's only pretty print in testing, not development.
let episodeAtomFeed = Html.link([ | ||
hasPodcastRssFeature | ||
? href(url(to: .feed(.episodes))) | ||
: href(url(to: .feed(.atom))), |
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.
feature flagging the feeds in the meta tags.
@@ -2,12 +2,12 @@ import Foundation | |||
|
|||
let introduction = Episode( | |||
blurb: """ | |||
Point-Free is here, bringing you videos covering functional programming concepts using the Swift language. | |||
Point-Free is here, bringing you videos covering functional programming concepts using the Swift language. \ |
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.
best to not have newlines for episode blurbs cause some readers will render them
assertSnapshot(matching: result.perform()) | ||
} | ||
|
||
func testEpisodeFeed_WithRecentlyFreeEpisode() { |
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 test shows that an older episode that was recently made free gets bumped to the top
en-US | ||
</language> | ||
<description> | ||
‼️ The URL for this feed has been turned off by Point-Free. You can retrieve your most up-to-date private podcast URL by visiting your account page at http://localhost:8080/account. If you think this is an error, please contact support@pointfree.co. |
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 is what the feed looks like for non-subscribers / in-active subscribers / people with bad feed urls.
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.
All looks good! Was scared by the diff stats before realizing it was a snap :)
* wip * wip * wip * alpha * wip * some more tests * wip * wip * remove newlines from blurbs * add another test * update snaps * better explanations * clean up * clean up * time interval guid
* wip * wip * wip * alpha * wip * some more tests * wip * wip * remove newlines from blurbs * add another test * update snaps * better explanations * clean up * clean up * time interval guid Former-commit-id: 33fd8a3
This should be everything we need to launch the feature, other than some data entry.