Skip to content

announce about progress on section progress page#11976

Merged
Bjvanminnen merged 3 commits into
stagingfrom
progressAnnouncement
Nov 18, 2016
Merged

announce about progress on section progress page#11976
Bjvanminnen merged 3 commits into
stagingfrom
progressAnnouncement

Conversation

@Bjvanminnen

Copy link
Copy Markdown
Contributor

image

Show a message about progress on courses in which it's disabled. If you change courses in the dropdown the message will hide as appropriate.

Comment thread pegasus/helpers/section_api_helpers.rb Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to be able to know whether to show this message or not on a per course basis. This means we need to get the gatekeeper settings for each course on page load. Not sure if this ends up being more querying than we'd like.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ashercodeorg seems like someone who might have opinions on this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAIK, querying the Gatekeeper is cheap. Perhaps @wjordan can say for certain.

Comment thread pegasus/helpers/section_api_helpers.rb Outdated

def self.progress_disabled_courses(user_id = nil)
disabled_courses = valid_courses(user_id).select do |course|
script_name = Script.find(course[:id]).name

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason not to use the script cache (via Script.get_from_cache(course[:id]).name)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nope. will fix

end
end

def self.progress_disabled_courses(user_id = nil)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please document the method: what does it do, what does it return, etc.

@ashercodeorg

Copy link
Copy Markdown
Contributor

LGTM, though you should get someone else to review the non-ruby code.

@Bjvanminnen Bjvanminnen merged commit 797e55e into staging Nov 18, 2016
@Bjvanminnen Bjvanminnen deleted the progressAnnouncement branch November 18, 2016 19:46
@Bjvanminnen

Copy link
Copy Markdown
Contributor Author

Script.* doesn't work on pegasus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants