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

Added data as optional JSONField to extra price models. #216

Merged
merged 7 commits into from Mar 11, 2013

Conversation

jrief
Copy link
Member

@jrief jrief commented Feb 26, 2013

Cart modifiers can add an optional data field beside label and value for both, the ExtraOrderPriceField and the ExtraOrderItemPriceField model.
This extra data field can contain anything, which is serializable as JSON.

This is based on issue #139 which however used a TextField for the data field. Since additional data can have any format, this pull request uses a JSONfield. This avoids additional string-to-object conversions.

@bmihelac
Copy link
Contributor

@jrief, requirements should be added to `ìnstall_requires``:

https://github.com/divio/django-shop/blob/master/setup.py#L27

Otherwise, I am for using json field instead of text field too.

@chrisglass
Copy link
Contributor

So I found the problem:

Previous versions (the versions that "work") simply pass in case of ValueError... Newer versions raise a ValidationError instead (which is desirable - it is not valid JSON).

I think we should simply not allow plain ASCII, it's a JSON field so we should have at least one key/value. Having a hybrid approach seems a little unclean: either it's structured data or it's not, but not "both"...

So I think we should not allow the case of the failing test (we should just enforce data to be JSON if it goes into the JSON field).

@jrief
Copy link
Member Author

jrief commented Feb 26, 2013

The maintainer of jsonfield also agreed on this, see rpkilby/jsonfield#33.
I made a proposal on how to solve it. Maybe its solvable there.

@jrief
Copy link
Member Author

jrief commented Mar 10, 2013

jsonfield is available in version 0.9.6 which fixes the pending issue, all unit tests pass.
Now, this pull request is ready for merge.

@alesdotio
Copy link
Contributor

Nice! I'll try to test this on some real shops soon. I would also like to implement something similar for addresses on the order model #218

@chrisglass
Copy link
Contributor

@jrief could you please add >=0.9.6 to the jsonfield dependency? Apart from that, I'm ok to merge.

@jrief
Copy link
Member Author

jrief commented Mar 11, 2013

@chrisglass done: jsonfield>=0.9.6

@chrisglass
Copy link
Contributor

Yay! Finally merging requested feature number 1 :)

chrisglass added a commit that referenced this pull request Mar 11, 2013
Added ``data`` as optional JSONField to extra price models.
@chrisglass chrisglass merged commit 7e094c4 into awesto:master Mar 11, 2013
@jrief jrief deleted the add-data-to-extra-price-fields branch November 10, 2015 08:31
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.

None yet

4 participants