Skip to content

Conversation

@chrisrossi
Copy link
Contributor

This work is paid for by MIT and falls under their contributor's agreement.

@chrisrossi
Copy link
Contributor Author

Quick heads up to @ichuang and @pdpinch that this pull request has been issued.

@sarina
Copy link
Contributor

sarina commented Nov 15, 2013

Hi @chrisrossi what's the plan with this PR? It's been untouched for 21 days with no reviews. We're trying really hard to have all open PRs be PRs in progress, and to close PRs that still need work or that aren't planning on being merged soon.

@ichuang
Copy link
Contributor

ichuang commented Nov 15, 2013

Waiting for edx review and merge. MIT is happy with it.
On Nov 15, 2013 5:28 PM, "Sarina Canelake" notifications@github.com wrote:

Hi @chrisrossi https://github.com/chrisrossi what's the plan with this
PR? It's been untouched for 21 days with no reviews. We're trying really
hard to have all open PRs be PRs in progress, and to close PRs that still
need work or that aren't planning on being merged soon.


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

@sarina
Copy link
Contributor

sarina commented Nov 15, 2013

Yep, can some edX employees be tagged in the PR? If you don't know who to tag please indicate what team(s) might have interest and I can try to assign people appropriately.

@sarina
Copy link
Contributor

sarina commented Nov 15, 2013

Also, is this a dupe of #1338 ? if so, that should be noted in the description and the outdated PR should be closed.

@ichuang
Copy link
Contributor

ichuang commented Nov 15, 2013

This PR provides a feature for residential edX-platform users, and is not
meant for MOOC providers. Thus, perhaps folks on the Stanford team would
be naturally interested (@jbau?)

#1388 is not a dup - that PR goes to the MIT fork.

On Fri, Nov 15, 2013 at 5:53 PM, Sarina Canelake
notifications@github.comwrote:

Also, is this a dupe of #1338https://github.com/edx/edx-platform/pull/1338? if so, that should be noted in the description and the outdated PR should
be closed.


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

@sarina
Copy link
Contributor

sarina commented Nov 15, 2013

@ichuang great, thanks for the clarification!

For future PRs (and possibly an update to this PR), @chrisrossi can you please do the following:

  • In the PR description, please be as verbose as possible explaining what the change is. This helps us so much in contextualizing your PR and providing reviewers. Take a look at django-admin command for enabling email per course #1322 for an example of a verbose PR description for a new feature.
  • As far as code goes, a first pass is to make sure that your code is of high quality. This means ensuring plenty of comments, as well as a 100% pass rate when you run rake quality locally.
  • Please also make an entry in CHANGELOG describing your change (if it is big enough - eg a major bugfix, new feature, or update to existing functionality). Be sure to also indicate what system (LMS, CMS, etc) the your change affects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we have multiple definitions of 'due' here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@chrisndodge
Copy link
Contributor

Just noticing a few I18N questions. This PR seems to have a mixture of I18N ready and non-I18N code. I see some display strings are for API responses, so I'm not clear on our policy here, but it couldn't hurt to make I18N ready.

@sarina do you have an idea on who would be best to go through this. You're doing a good job looking out for languishing PR's, but I'm wondering if someone else might be able to take this over.

@sarina
Copy link
Contributor

sarina commented Dec 3, 2013

@chrisndodge we're discussing in the LMS room right now. I can take a look and possibly @ormsbee as well

@sarina
Copy link
Contributor

sarina commented Dec 3, 2013

@chrisrossi we're planning to look at this branch in the next two weeks. Could you please rebase your branch and respond to @chrisndodge's comments before the start of next week?

@sarina
Copy link
Contributor

sarina commented Dec 6, 2013

@chrisrossi also before we can review this, can you please update the description of this pull request to explain what this feature actually is?

@chrisrossi
Copy link
Contributor Author

This is in my work queue. It may be a couple of weeks, but I will get to
it. Jazkarta is in communication with MIT about this work.

Thanks,
Chris

On Fri, Dec 6, 2013 at 11:10 AM, Sarina Canelake
notifications@github.comwrote:

@chrisrossi https://github.com/chrisrossi also before we can review
this, can you please update the description of this pull request to explain
what this feature actually is?


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

@sarina
Copy link
Contributor

sarina commented Dec 6, 2013

Ah. We were planning to begin reviewing this PR next week, but we cannot effectively review if you cannot commit time to it.

If you're not going to work on this for a few weeks, please close this PR and re-open it when you are ready to have review and respond to the comments. Don't open until you have responded to the existing comments & rebased; write a good description of what feature you're adding and tag me.

Thanks.

@chrisrossi chrisrossi closed this Dec 6, 2013
@sarina
Copy link
Contributor

sarina commented Dec 13, 2013

Superseded by #1928

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Dec 21, 2016
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.

4 participants