Skip to content

Conversation

@dmitchell
Copy link
Contributor

I didn't change places which aren't using expect_json to use it, but I did fix expect_json and all the unit tests afaik. It's interesting that our existing unit tests were avoiding the json serialization deserialization by never using "application/json" for the ACCEPT but instead using "multipart/form"

@singingwolfboy @cahrens please review

@singingwolfboy
Copy link
Contributor

@dmitchell Jenkins is showing test failures on this PR, but I don't know if those failures are due to your code changes or not. Can you take a look, fix if necessary, rebase onto latest master, and re-run the tests?

Copy link

Choose a reason for hiding this comment

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

When does one use _replace vs. replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For locations always use replace since chris created a method which stifles the pylint error of using an internal fn.

@singingwolfboy
Copy link
Contributor

Seems like it would be a good idea to subclass the Django test client and add a new method:

class AjaxEnabledTestClient(Client):
    def ajax_post(self, path, data={}, content_type="application/json", **kwargs):
        if not isinstance(data, basestring):
            data = json.dumps(data)
        kwargs.setdefault("HTTP_X_REQUESTED_WITH", "XMLHttpRequest")
        return self.post(path=path, data=data, content_type=content_type, **kwargs)

Then attach this subclass to the test suite as self.client, and call self.client.ajax_post() where appropriate. That way, this logic isn't duplicated many times, and it's clearer what you're trying to achieve. (You may also want to add an ajax_get method, but that seems less used (if at all).

@dmitchell
Copy link
Contributor Author

Passes all the tests now.

@cahrens
Copy link

cahrens commented Oct 29, 2013

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

$.postJSON and $["postJSON"] are exactly equivalent in Javascript, and the former is easier to read.

@singingwolfboy
Copy link
Contributor

👍

dmitchell added a commit that referenced this pull request Oct 29, 2013
Change expect_json to put parsed json in new attr
@dmitchell dmitchell merged commit 80c83f0 into master Oct 29, 2013
@dmitchell dmitchell deleted the dhm/expect_json branch October 29, 2013 21:02
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Dec 21, 2016
* Fix enrollment_date for paid course openedx#1240

* Implements ga-self-paced for Studio openedx#1240

* Implements ga-self-paced courseware openedx#1240

* Fix review
xirdneh pushed a commit to open-craft/openedx-platform that referenced this pull request Jul 15, 2019
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