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.

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

Feature idde #1928

Closed
wants to merge 34 commits into from
Closed

Feature idde #1928

wants to merge 34 commits into from

Conversation

chrisrossi
Copy link
Contributor

This changeset adds a feature to the edX platform which allows instructors to set individual due dates for students on particular coursework. This code is meant primarily for on-campus use--it is not intended that this feature would be used for MOOCs. It adds a new tab, "Extensions", to the instructor dashboard which allows changing due dates per student. This feature is enabled by setting FEATURES['INDIVIDUAL_DUE_DATES'] = True.

This work is performed on behalf of MIT and falls under the contributor agreement.

Chris Rossi and others added 26 commits October 3, 2013 12:17
…friendly error message if user fails to do so.
dashboard code i18n, since the existing legacy dashboard code is not i18n.
@pdpinch
Copy link
Contributor

pdpinch commented Dec 12, 2013

Thanks @chrisrossi Can you squash your commits (Preferably with all the messages so we can maintain the commit messages for @carsongee).

We've asked @sarina to get this into Jenkins. If the tests are good, we'll tag some other folks at edX for review.

@chrisrossi
Copy link
Contributor Author

Hey Peter,

I had trouble using the rebase command with this one--failed multiple times
in ways I couldn't recover from. I wound up having to do a merge from
master instead to get things consistent. I don't know how a squash would
work in that scenario. I understand the desire to squash, but I think I'd
prefer to play it safe for now and leave as-is.

Thanks,
Chris

On Thu, Dec 12, 2013 at 9:54 AM, Peter Pinch notifications@github.comwrote:

Thanks @chrisrossi https://github.com/chrisrossi Can you squash your
commits (Preferably with all the messages so we can maintain the commit
messages for @carsongee https://github.com/carsongee).

We've asked @sarina https://github.com/sarina to get this into Jenkins.
If the tests are good, we'll tag some other folks at edX for review.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/1928#issuecomment-30428585
.

@sarina
Copy link
Contributor

sarina commented Dec 12, 2013

The tests have passed, and we've whitelisted the jazkarta org to get builds built.

@chrisrossi you can interactively rebase locally to try to squash commits; if something goes wrong you can simply run git reset --hard HEAD to reset your local branch to reflect what's pushed up to github. If the interactive rebasing goes well, then run all the tests locally, and if they pass, then finally force-push the squashed branch up to github.

Or you can copy this branch to a new branch and do the rebasing, if you're really worried about things going horribly wrong and want to ensure things don't mess up.

@chrisrossi
Copy link
Contributor Author

Sorry, but rebase just doesn't work on this branch, probably because of the way it was merged earlier. It should merge just fine to master though. I think the only avenue left for truly rebasing will involve starting a new branch and cherry picking commits one by one, resolving conflicts, and testing again at the end. If squashing commits is worth that level of extra effort to you guys, I'm happy to do it. Just let me know.

@chrisrossi
Copy link
Contributor Author

@sarina Yeah, one of the big hits on test coverage is the legacy.py stuff. At first blush, I thought you guys weren't testing legacy.py, so I didn't either. It turns out I was just looking in the wrong place for the tests. Another cultural difference.

@sarina
Copy link
Contributor

sarina commented Dec 18, 2013

@chrisrossi we've actually taken to simply not putting new features on the legacy dashboard. We plan to deprecate the legacy dash (hide behind a feature flag) but still have it in our codebase for other openedx'ers who may still have it in use.

@chrisrossi
Copy link
Contributor Author

@sarina Would you prefer that I just remove the feature from the legacy dashboard, then?

@sarina
Copy link
Contributor

sarina commented Dec 18, 2013

@chrisrossi I have no strong preference, I think it is probably more up to how the feature is being used at MIT and how jarring it would be to force change the interface. If it remains on the legacy dashboard that's fine but it needs to be tested. If you remove it from the legacy dash, you don't need to test it there.

@chrisrossi
Copy link
Contributor Author

@pdpinch It's your call. Should we continue to invest in the legacy dashboard?

@pdpinch
Copy link
Contributor

pdpinch commented Dec 19, 2013

In the interests of time and effort, lets drop the legacy dashboard.

We'll be able to direct any instructors who need this feature to the beta dashboard.

fyi, @ichuang, @carsgongee

On Dec 18, 2013, at 2:09 PM, Chris Rossi wrote:

@pdpinchhttps://github.com/pdpinch It's your call. Should we continue to invest in the legacy dashboard?


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/1928#issuecomment-30870645.

return None
elif not extended or extended < due_date:
return due_date
return extended
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ever possible to get into a state where there is an extended, but due_date is None? I ask because I think that'll cause an error when trying to do the extended < due_date comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UI only allows extensions on modules with due dates. So, it is possible, but it would require manually monkeying with the database.

@sarina
Copy link
Contributor

sarina commented Dec 20, 2013

@chrisrossi at some point this'll need a rebase, as the PR can't be automagically merged.

@sarina
Copy link
Contributor

sarina commented Dec 20, 2013

Also @chrisrossi I'm off for most of the next two weeks. Could you or @pdpinch or @carsongee let me know what your availability is and when you'd like to get this merged in by? If you're going to be around the first full week of January, I'd like to table my review until then; however if time is short for you I can make time the week of new year's.

@sarina
Copy link
Contributor

sarina commented Dec 20, 2013

Is there something special I need to do to get this working locally? I have two graded subsections in my course, each with two questions, but when I go to "Choose Graded Unit" nothing pops up.

Here's the Studio portion of my test course (two graded units):

screen shot 2013-12-20 at 3 37 40 pm

Here's the dashboard:

screen shot 2013-12-20 at 3 37 06 pm

As a side note I'm going to want the UX here changed, the flow is a bit nonsensical. I'll post a mockup in a bit.

@sarina
Copy link
Contributor

sarina commented Dec 20, 2013

Arugh. Last post. I see the constraints you're under here on the instructor dashboard, and I want to try to make the page easy to understand and have a better flow without having the same boxes over and over if possible. I'd like to run a few of my mocks by the UX team before I decide what to have you implement. Can that wait until Jan 6th?

There's definitely an "ideal" UX, but I'm striving for something that's cleaner than the current dashboard without a whole lot of code changing (the "ideal" probably being way too much effort).

@chrisrossi
Copy link
Contributor Author

On Fri, Dec 20, 2013 at 6:38 PM, Sarina Canelake
notifications@github.comwrote:

Is there something special I need to do to get this working locally? I
have two graded subsections in my course, each with two questions, but when
I go to "Choose Graded Unit" nothing pops up.

You just need to set some due dates. Units with due dates will show up.

Chris

@chrisrossi
Copy link
Contributor Author

I will be taking some time off for the holidays, but will be working on
most days that aren't obviously holidays. My plan is to take some time
next week to address the issues raised so far on this PR.

On Fri, Dec 20, 2013 at 6:17 PM, Sarina Canelake
notifications@github.comwrote:

Also @chrisrossi https://github.com/chrisrossi I'm off for most of the
next two weeks. Could you or @pdpinch https://github.com/pdpinch or
@carsongee https://github.com/carsongee let me know what your
availability is and when you'd like to get this merged in by? If you're
going to be around the first full week of January, I'd like to table my
review until then; however if time is short for you I can make time the
week of new year's.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/1928#issuecomment-31048999
.

@chrisrossi
Copy link
Contributor Author

@sarina Changing the UI puts me in an awkward position. I've been asked to work on this for MIT, who wants to have the feature, but doesn't have unlimited budget to have the feature implemented. It seems like the finish line for this feature keeps changing. It's an odd position to be in where the folks changing the scope of the work aren't the same folks that are paying for it. I think we'll need for edX to confer with MIT (@pdpinch) about what is in and out of scope for this feature. I'm happy to keep going as long as you guys are in agreement about what needs to be done.

@sarina
Copy link
Contributor

sarina commented Dec 21, 2013

Oh I totally understand that. I just want more explanatory text around the
buttons and maybe a different layout. Just visual rearrangement not
reimplementation of anything! But what that text should say is something I
always run bu the ux people.
On Dec 20, 2013 5:01 PM, "Chris Rossi" notifications@github.com wrote:

@sarina https://github.com/sarina Changing the UI puts me in an awkward
position. I've been asked to work on this for MIT, who wants to have the
feature, but doesn't have unlimited budget to have the feature implemented.
It seems like the finish line for this feature keeps changing. It's an odd
position to be in where the folks changing the scope of the work aren't the
same folks that are paying for it. I think we'll need for edX to confer
with MIT (@pdpinch https://github.com/pdpinch) about what is in and out
of scope for this feature. I'm happy to keep going as long as you guys are
in agreement about what needs to be done.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/1928#issuecomment-31052814
.

@sarina
Copy link
Contributor

sarina commented Dec 21, 2013

Worst case I can commit the necessary few changes.
On Dec 20, 2013 5:22 PM, sarina@edx.org wrote:

Oh I totally understand that. I just want more explanatory text around the
buttons and maybe a different layout. Just visual rearrangement not
reimplementation of anything! But what that text should say is something I
always run bu the ux people.
On Dec 20, 2013 5:01 PM, "Chris Rossi" notifications@github.com wrote:

@sarina https://github.com/sarina Changing the UI puts me in an
awkward position. I've been asked to work on this for MIT, who wants to
have the feature, but doesn't have unlimited budget to have the feature
implemented. It seems like the finish line for this feature keeps changing.
It's an odd position to be in where the folks changing the scope of the
work aren't the same folks that are paying for it. I think we'll need for
edX to confer with MIT (@pdpinch https://github.com/pdpinch) about
what is in and out of scope for this feature. I'm happy to keep going as
long as you guys are in agreement about what needs to be done.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/1928#issuecomment-31052814
.

@chrisrossi
Copy link
Contributor Author

Ah, ok. Minor tweaks are probably ok, as long as we stay in budget. Thanks!

@chrisrossi
Copy link
Contributor Author

Superceded by #2062.

@chrisrossi chrisrossi closed this Dec 31, 2013
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jun 23, 2017
* Fix review, add bok-choy.  openedx#1867

* Fix bug for login page message. openedx#1932
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.

6 participants