186 session subscriberid #188
Conversation
My local test runs started crashing Metaspace OutOfMemory errors when fork was disabled in the Test environment. Hopefully this doesn’t reintroduce Travis instability which was the reason this was changed initially in 7296816
- In order to move to storing a SubscriberId in the session we need to move the test Env authentication closer to how production works. This is actually win/win as now we are testing more of the production authentication path as a side effect of this work. Yay. - app/services/OAuth2.scala#currentSubscriber is really just a stub for future functionality in which we’ll need to figure out a way to handle Users with multiple Subscribers.
- This has worked for a long time, but we had no test as we bypassed this portion of the code in the Test ENV. Now that we use more of the true Authentication path in the Test ENV this is testable.
We need to access some session variables in Views as well as in the main Layout view (Navigation in particular). The implicit RequestHeader allows for that and is the recommended way as of Play 2.3.x instead of accessing the session directly. If we just needed Session data it in some views, we could pass it to them via the Controller explicitly. However, since we also need it in the main Layout template, we seemingly need to implicitly pass it to each view and have those views implicitly pass it to the Layout. Or at least that’s how I solved this.
|
FWIW: the Travis build crashed since I reenabled test in Fork which I needed for local development. There is an open Issue with the Travis team on this and they suggested trying the old architecture by setting |
Switch to old Travis infrastructure
|
2fcf82e merged into this PR a Travis setting to use the old infrastructure (non container based) which does seem to be a workaround for now at least. |
| @@ -365,6 +355,7 @@ object Application extends Controller with Security { | |||
| } | |||
|
|
|||
| def scheme(id: Int) = isAnalyst { identity => implicit request => | |||
| // todo: update to currentSubscriberId | |||
There was a problem hiding this comment.
Oops, missed one. I'll add a commit to this PR.
There was a problem hiding this comment.
Umm, I might want to rethink this. Right now the scheme page is where subscribers add interests, but generally its where users can learn about the scheme.
I don’t see an analyst restriction (although there are analyst-specific functions that might link from here)
RIchard
On May 1, 2015, at 10:05 AM, Jeremy Prevost notifications@github.com wrote:
In app/controllers/Application.scala:
@@ -365,6 +355,7 @@ object Application extends Controller with Security {
}def scheme(id: Int) = isAnalyst { identity => implicit request =>
- // todo: update to currentSubscriberId
Oops, missed one. I'll add a commit to this PR.—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
I noticed that as well just now. I think until you added Interests 2.0 to that page it seemed like just a page with the Scheme name and link to adminy things so the isAnalyst restriction made sense to me. With that being the only entry point to add a new Interest, we do indeed need a ticket to update the required role.
This just updates this method that was missed in the previous work for this PR.
|
👍 for merging. I would like in time to begin a refactor over test code so that the browser login actions are abstracted somewhat, so we can change the mechanism without touching all the test classes, but that and other abstractions should be tackled in a different thread/set of issues |
|
We can probably clean the tests up with Traits... but yeah, another day. |
Please see individual commit messages for some details on this PR.
Closes #185