Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update user_progress and user_app_options to support teacher view for cached levels #43422

Merged
merged 2 commits into from
Nov 10, 2021
Merged

Update user_progress and user_app_options to support teacher view for cached levels #43422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4282426
Update user_progress and user_app_options to support teacher view for…
jamescodeorg Nov 5, 2021
7aa8d4d
Add/update unit tests
jamescodeorg Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 21 additions & 8 deletions apps/src/code-studio/initApp/loadApp.js
View file
Open in desktop
Expand Up @@ -299,14 +299,18 @@ async function loadAppAsync(appOptions) {
appOptions.disableSocialShare = true;
}

const userAppOptionsRequest = $.ajax(
`/api/user_app_options` +
const userAppOptionsRequest = $.ajax({
url:
`/api/user_app_options` +
`/${appOptions.scriptName}` +
`/${appOptions.lessonPosition}` +
`/${appOptions.levelPosition}` +
`/${appOptions.serverLevelId}` +
`?get_channel_id=${shouldGetChannelId}`
);
`/${appOptions.serverLevelId}`,
data: {
user_id: clientState.queryParams('user_id'),
get_channel_id: shouldGetChannelId
}
});

const sectionId = clientState.queryParams('section_id') || '';
const exampleSolutionsRequest = $.ajax(
Expand All @@ -324,9 +328,18 @@ async function loadAppAsync(appOptions) {
appOptions.exampleSolutions = exampleSolutions;
appOptions.disableSocialShare = data.disableSocialShare;

// We do not need to process data.progress here because labs do not use
// the level progress data directly. (The progress bubbles in the header
// of the level pages are rendered by header.build in header.js.)
if (data.isStarted) {
appOptions.level.isStarted = data.isStarted;
}
if (data.skipInstructionsPopup) {
appOptions.level.skipInstructionsPopup = data.skipInstructionsPopup;
}
if (data.readonlyWorkspace) {
jamescodeorg marked this conversation as resolved.
Show resolved Hide resolved
appOptions.readonlyWorkspace = data.readonlyWorkspace;
}
if (data.callouts) {
appOptions.callouts = data.callouts;
}

if (data.lastAttempt) {
appOptions.level.lastAttempt = data.lastAttempt.source;
Expand Down
5 changes: 4 additions & 1 deletion apps/src/code-studio/progress.js
View file
Open in desktop
Expand Up @@ -197,7 +197,10 @@ function getLevelProgress(signedIn, progressData, scriptName) {
case null:
// We do not know if user is signed in or not, send a request to the server
// to find out if the user is signed in and retrieve progress information
return $.ajax(`/api/user_progress/${scriptName}`)
return $.ajax({
url: `/api/user_progress/${scriptName}`,
data: {user_id: clientState.queryParams('user_id')}
jamescodeorg marked this conversation as resolved.
Show resolved Hide resolved
})
.then(data => {
if (data.signedIn) {
return {
Expand Down
79 changes: 28 additions & 51 deletions apps/test/unit/code-studio/initApp/loadAppTest.js
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@ describe('loadApp.js', () => {
sinon.stub(project, 'getSharingDisabled').returns(false);
});
beforeEach(() => {
sinon.stub(clientState, 'queryParams').returns(undefined);
sinon.stub($, 'ajax').callsFake(() => ({
done: successCallback => ({
fail: failureCallback => {
Expand Down Expand Up @@ -66,38 +67,26 @@ describe('loadApp.js', () => {
window.appOptions = oldAppOptions;
});
afterEach(() => {
clientState.queryParams.restore();
$.ajax.restore();
});

const stubQueryParams = (paramName, paramValue) => {
clientState.queryParams.restore(); // restore the default stub
sinon
.stub(clientState, 'queryParams')
.callsFake(param => (param === paramName ? paramValue : undefined));
};

const stubAppOptionsRequests = (
appOptions,
userAppOptionsResponse = {signedIn: false},
exampleSolutionsResponse = []
) => {
const {
scriptName,
lessonPosition,
levelPosition,
serverLevelId,
serverScriptLevelId,
levelRequiresChannel,
channel
} = appOptions;

$.ajax.restore();
const ajaxStub = sinon.stub($, 'ajax');
ajaxStub
.withArgs(
`/api/user_app_options/${scriptName}/${lessonPosition}/${levelPosition}/${serverLevelId}?get_channel_id=${!!levelRequiresChannel &&
!channel}`
)
.returns(userAppOptionsResponse);

ajaxStub
.withArgs(
`/api/example_solutions/${serverScriptLevelId}/${serverLevelId}?section_id=`
)
.returns(exampleSolutionsResponse);
ajaxStub.onCall(0).returns(userAppOptionsResponse);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path-specific stubbing wasn't working because different tests use different query parameters. This is a temporary fix so that we can get this merged and unblock the next set of work. If it's ok with you, I'd like to merge this and then clean-up these tests in a follow-up PR.

ajaxStub.onCall(1).returns(exampleSolutionsResponse);
};

it('loads attempt stored under server level id', done => {
Expand Down Expand Up @@ -133,21 +122,13 @@ describe('loadApp.js', () => {
const appOptionsData = document.createElement('script');
appOptionsData.setAttribute('data-appoptions', JSON.stringify(appOptions));
document.body.appendChild(appOptionsData);
const queryParamsStub = sinon
.stub(clientState, 'queryParams')
.callsFake(param => {
if (param === 'solution') {
return 'true';
}
return undefined;
});
stubQueryParams('solution', 'true');

loadAppOptions().then(() => {
expect(window.appOptions.level.lastAttempt).to.be.undefined;
expect(readLevelId).to.be.undefined;

document.body.removeChild(appOptionsData);
queryParamsStub.restore();
done();
});
});
Expand All @@ -156,23 +137,17 @@ describe('loadApp.js', () => {
const appOptionsData = document.createElement('script');
appOptionsData.setAttribute('data-appoptions', JSON.stringify(appOptions));
document.body.appendChild(appOptionsData);
const queryParamsStub = sinon
.stub(clientState, 'queryParams')
.callsFake(param => {
if (param === 'user_id') {
return 'true';
}
return undefined;
});
stubQueryParams('user_id', '12345');

loadAppOptions().then(() => {
expect(window.appOptions.level.lastAttempt).to.be.undefined;
expect(readLevelId).to.be.undefined;
loadAppOptions()
.then(() => {
expect(window.appOptions.level.lastAttempt).to.be.undefined;
expect(readLevelId).to.be.undefined;

document.body.removeChild(appOptionsData);
queryParamsStub.restore();
done();
});
document.body.removeChild(appOptionsData);
done();
})
.catch(err => done(err));
});

it('passes get_channel_id true to load user progress if appOptions has levelRequiresChannel true and no channel', done => {
Expand All @@ -196,11 +171,13 @@ describe('loadApp.js', () => {
appOptionsData.setAttribute('data-appoptions', JSON.stringify(appOptions));
document.body.appendChild(appOptionsData);

loadAppOptions().then(() => {
expect(window.appOptions.channel).to.equal(responseChannel);
document.body.removeChild(appOptionsData);
done();
});
loadAppOptions()
.then(() => {
expect(window.appOptions.channel).to.equal(responseChannel);
document.body.removeChild(appOptionsData);
done();
})
.catch(err => done(err));
});

it('calls example_solutions endpoint and sets example solutions to appOptions', done => {
Expand Down
10 changes: 0 additions & 10 deletions dashboard/app/controllers/activities_controller.rb
View file
Open in desktop
Expand Up @@ -131,16 +131,6 @@ def milestone
user_level: @user_level
)

if solved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious what this was for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for the "activity monitor". See this slack thread for more info on why it's no longer needed.

slog(
tag: 'activity_finish',
script_level_id: @script_level.try(:id),
level_id: @level.id,
user_agent: request.user_agent,
locale: locale
)
end

# log this at the end so that server errors (which might be caused by invalid input) prevent logging
log_milestone(@level_source, params)
end
Expand Down
115 changes: 75 additions & 40 deletions dashboard/app/controllers/api_controller.rb
View file
Open in desktop
Expand Up @@ -425,8 +425,14 @@ def script_standards
# Return a JSON summary of the user's progress for params[:script].
def user_progress
if current_user
if params[:user_id].present?
user = User.find(params[:user_id])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we guard against ActiveRecord::RecordNotFound here? Or replace with find_by or some other method which will return nil if record couldn't be found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It's intentional that we throw an exception here so we don't continue execution here (same as the previous behavior). As written, if the request has an invalid user_id, the whole request will fail with a 404 error. I'm not sure if this is the best error to return and we also don't really handle this error on the client but this behavior is pre-existing and I'm inclined to solve that problem another day?

return head :forbidden unless user.student_of?(current_user)
else
user = current_user
end

script = Script.get_from_cache(params[:script])
user = params[:user_id].present? ? User.find(params[:user_id]) : current_user
teacher_viewing_student = !current_user.student? && current_user.students.include?(user)
render json: summarize_user_progress(script, user).merge(
{
Expand All @@ -444,66 +450,95 @@ def user_progress
# Returns app_options values that are user-specific. This is used on cached
# levels.
def user_app_options
response = user_summary(current_user)
response[:signedIn] = !current_user.nil?
response = {}

script = Script.get_from_cache(params[:script])
lesson = script.lessons[params[:lesson_position].to_i - 1]
script_level = lesson.cached_script_levels[params[:level_position].to_i - 1]
level = params[:level] ? Script.cache_find_level(params[:level].to_i) : script_level.oldest_active_level

if current_user
user_level = current_user.last_attempt(level, script)
level_source = user_level.try(:level_source).try(:data)

# Temporarily return the full set of progress so we can overwrite what the sessionStorage changed
response[:progress] = summarize_user_progress(script, current_user)[:progress]
response[:isHoc] = script.hoc?

if user_level
response[:lastAttempt] = {
timestamp: user_level.updated_at.to_datetime.to_milliseconds,
source: level_source
}
response[:signedIn] = true

# Set `user` to the user that we should use to calculate the app_options
# and note if the signed-in user is viewing another user (e.g. a teacher
# viewing a student's work).
if params[:user_id].present?
user = User.find(params[:user_id])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here about checking for ActiveRecord::RecordNotFound

return head :forbidden unless can?(:view_as_user, script_level, user)
viewing_other_user = true
else
user = current_user
viewing_other_user = false
end

# Pairing info
is_navigator = user_level.navigator?
if is_navigator
driver = user_level.driver
driver_level_source_id = user_level.driver_level_source_id
end
if viewing_other_user
response[:isStarted] = level_started?(level, script, user)

response[:isNavigator] = is_navigator
if driver
response[:pairingDriver] = driver.name
if driver_level_source_id
response[:pairingAttempt] = edit_level_source_path(driver_level_source_id)
elsif level.channel_backed?
response[:pairingChannelId] = get_channel_for(level, script.id, driver)
end
end
# This is analogous to readonly_view_options
response[:skipInstructionsPopup] = true
response[:readonlyWorkspace] = true
response[:callouts] = []
end
end

if level.finishable?
slog(
tag: 'activity_start',
script_level_id: script_level.try(:id),
level_id: level.contained_levels.empty? ? level.id : level.contained_levels.first.id,
user_agent: request.user_agent&.valid_encoding? ? request.user_agent : 'invalid_encoding',
locale: locale
)
# TODO: There are many other user-specific values in app_options that may
# need to be sent down. See LP-2086 for a list of potential values.

response[:disableSocialShare] = !!user.under_13?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why the !! is needed, under_13? seems to imply it returns a boolean from the ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch. I wonder why I did that. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pick up this change in my next PR.

response.merge!(progress_app_options(script, level, user))
else
response[:signedIn] = false
viewing_other_user = false
end

if params[:get_channel_id] == "true"
response[:channel] = get_channel_for(level, script.id, current_user)
response[:channel] = viewing_other_user ?
get_channel_for(level, script.id, user) :
get_channel_for(level, script.id)
response[:reduceChannelUpdates] =
!Gatekeeper.allows("updateChannelOnSave", where: {script_name: script.name}, default: true)
end

render json: response
end

# Gets progress-related app_options for the given script and level for the
# given user. This code is analogous to parts of LevelsHelper#app_options.
# TODO: Eliminate this logic from LevelsHelper#app_options or refactor methods
# to share code.
def progress_app_options(script, level, user)
response = {}

user_level = user.last_attempt(level, script)
level_source = user_level.try(:level_source).try(:data)

if user_level
response[:lastAttempt] = {
timestamp: user_level.updated_at.to_datetime.to_milliseconds,
source: level_source
}

# Pairing info
is_navigator = user_level.navigator?
if is_navigator
driver = user_level.driver
driver_level_source_id = user_level.driver_level_source_id
end

response[:isNavigator] = is_navigator
if driver
response[:pairingDriver] = driver.name
if driver_level_source_id
response[:pairingAttempt] = edit_level_source_path(driver_level_source_id)
elsif level.channel_backed?
response[:pairingChannelId] = get_channel_for(level, script.id, driver)
end
end
end

response
end

# GET /api/example_solutions/:script_level_id/:level_id
def example_solutions
script_level = Script.cache_find_script_level params[:script_level_id].to_i
Expand Down