-
Notifications
You must be signed in to change notification settings - Fork 482
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.
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
Hide course versions #26678
Hide course versions #26678
Changes from 9 commits
1b3c81a
3457983
172d75d
5f8e25d
be6ade3
7d6a45b
c2f55c8
329e761
b457210
21917b0
b7c4b70
a92097b
7d5ad3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,8 @@ export default class CourseOverview extends Component { | |
hasVerifiedResources: PropTypes.bool.isRequired, | ||
versions: PropTypes.arrayOf(PropTypes.shape({ | ||
name: PropTypes.string.isRequired, | ||
version_year: PropTypes.string.isRequired | ||
version_year: PropTypes.string.isRequired, | ||
can_view_version: PropTypes.bool.isRequired, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while you are in here, please add |
||
})).isRequired, | ||
showVersionWarning: PropTypes.bool, | ||
showRedirectWarning: PropTypes.bool, | ||
|
@@ -150,6 +151,9 @@ export default class CourseOverview extends Component { | |
const showNotification = viewAs === ViewType.Teacher && isTeacher && | ||
!isVerifiedTeacher && hasVerifiedResources; | ||
|
||
// Only display viewable versions in course version dropdown. | ||
const filteredVersions = versions.filter(version => version.can_view_version); | ||
|
||
return ( | ||
<div style={mainStyle}> | ||
{redirectToCourseUrl && !dismissedRedirectDialog(name) && | ||
|
@@ -181,7 +185,7 @@ export default class CourseOverview extends Component { | |
} | ||
<div style={styles.titleWrapper}> | ||
<h1 style={styles.title}>{assignmentFamilyTitle}</h1> | ||
{versions.length > 1 && | ||
{filteredVersions.length > 1 && | ||
<span style={styles.versionWrapper}> | ||
<span style={styles.versionLabel}>{i18n.courseOverviewVersionLabel()}</span> | ||
<select | ||
|
@@ -190,7 +194,7 @@ export default class CourseOverview extends Component { | |
style={styles.versionDropdown} | ||
id="version-selector" | ||
> | ||
{versions.map(version => ( | ||
{filteredVersions.map(version => ( | ||
<option key={version.name} value={version.name}> | ||
{version.version_title} | ||
</option> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,10 +121,10 @@ describe('CourseOverview', () => { | |
utils.navigateToHref.restore(); | ||
}); | ||
|
||
it('appears when two versions are present', () => { | ||
it('appears when two versions are present and viewable', () => { | ||
const versions = [ | ||
{name: 'csp', version_year: '2017'}, | ||
{name: 'csp-2018', version_year: '2018'}, | ||
{name: 'csp', version_year: '2017', can_view_version: true}, | ||
{name: 'csp-2018', version_year: '2018', can_view_version: true}, | ||
]; | ||
const wrapper = shallow( | ||
<CourseOverview | ||
|
@@ -143,9 +143,10 @@ describe('CourseOverview', () => { | |
expect(utils.navigateToHref).to.have.been.calledOnce; | ||
}); | ||
|
||
it('does not appear when only one version is present', () => { | ||
it('does not appear when only one version is viewable', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for keeping these test case names up to date! 💯 |
||
const versions = [ | ||
{name: 'csp', version_year: '2017'}, | ||
{name: 'csp', version_year: '2017', can_view_version: false}, | ||
{name: 'csp-2018', version_year: '2018', can_view_version: true}, | ||
]; | ||
const wrapper = shallow( | ||
<CourseOverview | ||
|
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 going to suggest that you extract a
scriptVersionShape
, shared between here and ScriptOverview. AscriptVersionShape
would contain all the information needed to display a row in the version dropdown on the script overview page. This will help make it clear that this PropType is distinct from the similar-but-differentassignmentVersionShape
.I'm also going to recommend that you do not (yet) share this shape with the
CourseOverview
. This is because course versions and script versions are generated in different places (see twosummarize_versions
methods), and we do want to create a hard dependency between these implementations.It may sound like I'm going off the deep end here re: not over-deduplicating. I blame these articles:
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 don't think you're going off the deep end! this is a good thing to DRY up