LevelSources are now shared with R links on the client #16777
Conversation
caleybrock
force-pushed
the
r-links-on-client
branch
from
e8b27a3
to
b805613
Compare
ashercodeorg
force-pushed
the
r-links-on-client
branch
from
2c9c662
to
6080804
Compare
| @@ -175,7 +175,7 @@ def milestone_response(options) | |||
| end | |||
|
|
|||
| if options[:level_source].try(:id) | |||
| response[:level_source] = level_source_url(id: options[:level_source].id) | |||
| response[:level_source] = obfuscated_level_source_url(level_source_id_and_user_id: options[:level_source].obfuscate_level_source_id(current_user.try(:id))) | |||
There was a problem hiding this comment.
This line is well wider than our 120 column styleguide limit. Please break this line up, e.g.
response[:level_source] = obfuscated_level_source_url(
level_source_id_and_user_id: options[:level_source].obfuscate_level_source_id(current_user.try(:id))
)
| # signed in. | ||
| def current_user_id | ||
| session['warden.user.user.key'].try(:first).try(:first) | ||
| end |
There was a problem hiding this comment.
Why not put it in test_helper now?
There was a problem hiding this comment.
Because I'd rather keep the PR changes self-contained. As part of moving it to test_helper, I'd want to convert other tests to make use of it. Another PR.
| @@ -409,7 +422,8 @@ def _test_logged_in_milestone_should_save_gallery_when_passing_an_impressive_lev | |||
|
|
|||
| assert_response :success | |||
|
|
|||
| expected_response = build_expected_response(level_source: "http://test.host/c/#{assigns(:level_source).id}") | |||
| obfuscated_level_source_url = assigns(:level_source).obfuscate_level_source_id(current_user_id) | |||
| expected_response = build_expected_response(level_source: "http://test.host/r/#{obfuscated_level_source_url}") | |||
There was a problem hiding this comment.
It seems like these 2 lines are repeated a lot. Would it make sense to consolidate them into a method?
There was a problem hiding this comment.
It would. That said, I'm calling that part of the "rewritten to be readable" in the PR summary, that I'm hoping to punt on as part of this PR. Expect a follow-up...
|
@poorvasingal last call to change the URL structure... Let us know if we are good on |
|
Poorva and Alice say use |
This PR replaces /c/ links with /r/ links in the UI of the share dialog.
The changes to
ActivitiesControllerTestare consistent with the existing hackiness in that file. The entire file needs to be rewritten to be readable. Being outside the scope of this PR, we are not doing so here.MANUAL TESTING (LOCALLY):
Paired with @ashercodeorg