-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
…that way we can for-yield over them and set a default behaviour.
… the cases where the query params are Empty.
@@ -80,7 +80,7 @@ object Joiner extends Controller with ActivityTracking | |||
contentURL <- contentRefererOpt if Uri.parse(contentURL).host.contains("www.theguardian.com") | |||
access <- accessOpt | |||
} yield { | |||
Redirect(routes.MemberOnlyContent.membershipContent(contentURL, access.name)) | |||
Redirect(routes.MemberOnlyContent.membershipContent(Some(contentURL))) |
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.
Probably legacy code at this point - is anyone still directing users to /tier-chooser as a content-access handler?
} | ||
}).getOrElse( | ||
Redirect(routes.Joiner.tierChooser()) | ||
Future(Redirect(("https://membership.theguardian.com/supporter"))) |
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.
Use the reverse-routing handler!
Ok(views.html.joiner.membershipContent(pageInfo, accessOpt, signInUrl, capiContent, s"Exclusive Members Content: $headline")). | ||
withSession(request.session + (DestinationService.JoinReferrer -> ("https://" + Config.guardianHost +"/" + referringContent))) | ||
} else { | ||
Redirect(("https://theguardian.com/" + referringContent)) | ||
} | ||
}).getOrElse( |
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 it worth considering fold? https://kwangyulseo.com/2014/05/21/scala-option-fold-vs-option-mapgetorelse/
import model._ | ||
import play.api.libs.concurrent.Execution.Implicits.defaultContext | ||
import play.api.mvc._ | ||
import services.{GuardianContentService, _} | ||
import views.support.PageInfo | ||
|
||
import scala.concurrent.Future |
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.
Unnecessary import?
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 am using Future in line 26
It is working on PROD. |
✅ Testing in PROD passed! Details |
Why are you doing this?
There are several errors in Sentry (here) about requests to /membership-content which are returning 400 Bad Request. The main reason for this is because the request doesn't have the necessary query parameters. At the same time, we spotted that are crawlers that are driven traffic to /membership-content also without the required parameters.
Therefore, in this PR we are deleting one of the parameters and marking the other one as optional and if we don't receive it, we direct that traffic to the supporter landing page.
Trello card: Here and Here
Changes
Screenshots
/membership-content
/membership-content?referringContent=membership/audio/2017/feb/02/we-need-to-talk-about-climate-change-guardian-members-exclusive-podcast
/membership-content?referringContent=membership/audio/2017/feb/02/we-need-to-talk-about-climate-change-guardian-members-exclusive-podcast&membershipAccess=MembersOnly