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

Incidental functionality improvements from Next Gen Modulestore work #269

Merged
merged 15 commits into from
Jul 1, 2013

Conversation

cpennington
Copy link
Contributor

@dmitchell: Please review content and add reviewers to validate changes.

This builds on top of #268 (net change: edx/edx-platform@dhm/cosmetic-cleanup...dhm/incidental-functionality-improvements)

@dmitchell
Copy link
Contributor

@cahrens @chrisndodge @jzoldak Please review (number 2 of 4)

@chrisndodge
Copy link
Contributor

Overall comment: It's unfortunate that we see the deltas which are in the 1st PR. Don suggested if we merge the first PR, those deltas will no longer appear in this PR. Is that true?

If so, can we merge the 1st PR so to clean up this 2nd PR. My understanding is that we're breaking up the PR's to make things much more atomic.

@cpennington
Copy link
Contributor Author

Yes, it's true. We definitely want to make things more atomic, but I didn't
want to try and make each PR off of master, since that would have
introduced more conflicts to resolve.

However, you can also look at the pure delta by clicking the link in the PR
to the github compare page.

On Wed, Jun 26, 2013 at 10:59 AM, chrisndodge [email protected]:

Overall comment: It's unfortunate that we see the deltas which are in the
1st PR. Don suggested if we merge the first PR, those deltas will no longer
appear in this PR. Is that true?

If so, can we merge the 1st PR so to clean up this 2nd PR. My
understanding is that we're breaking up the PR's to make things much more
atomic.


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

from json.encoder import JSONEncoder
import datetime

class EdxJSONEncoder(json.JSONEncoder):
Copy link

Choose a reason for hiding this comment

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

Unit test for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@chrisndodge
Copy link
Contributor

+1 but we must verify that deploying the config change will work against the previous code deploy, otherwise there will be downtime. My suggestion would be to do the config change on sandbox/stage ahead of code deploy and verify that things work (both draft/direct).

@cahrens
Copy link

cahrens commented Jun 30, 2013

I am ready to give a thumbs-up once there are negative tests for almost_same_datetime and Chris' question about config change is settled.

@cpennington
Copy link
Contributor Author

I have the negative tests. I just need to merge them. We need to do some
fixes to make the code backwards compatible with the prod config.

-Cale
On Jun 30, 2013 1:20 PM, "Christina Roberts" [email protected]
wrote:

I am ready to give a thumbs-up once there are negative tests for
almost_same_datetime and Chris' question about config change is settled.


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

cpennington added a commit that referenced this pull request Jul 1, 2013
…ments

Incidental functionality improvements from Next Gen Modulestore work
@cpennington cpennington merged commit 1a7b833 into master Jul 1, 2013
@cpennington cpennington deleted the dhm/incidental-functionality-improvements branch July 1, 2013 20:04
@cpennington cpennington mentioned this pull request Jul 2, 2013
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
Kelketek referenced this pull request in open-craft/edx-platform Oct 21, 2014
…de-complete-count

ziafazal/api-fix-users-grade-complete-count: fixed count exceeding total...
diegomillan referenced this pull request in eduNEXT/edx-platform Sep 14, 2016
prabhanshu pushed a commit to prabhanshu/edx-platform that referenced this pull request Oct 13, 2018
* Client credentials + grades api (openedx#228)

* Client credentials and grades api

* Updating the permissions on the fly for request object (openedx#233)

* Updating the permissions on the fly for request object

* Update views.py
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
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