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

#76 : edit forms #83

Merged
merged 9 commits into from
Apr 3, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/base/custom_models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from django.db import models
import ast


class ListField(models.Field):
description = "Stores a python list"

def __init__(self, *args, **kwargs):
super(ListField, self).__init__(*args, **kwargs)

def from_db_value(self, value, expression, connexion, context):
return self.to_python(value)

def to_python(self, value):
if not value:
value = []

if isinstance(value, list):
return value

return ast.literal_eval(value)

def get_db_prep_value(self, value, connection, prepared=False):
value = super().get_db_prep_value(value, connection, prepared)
return str(value)

def get_prep_value(self, value):
if value is None:
return value
return str(value)
28 changes: 28 additions & 0 deletions src/cards/migrations/0010_auto_20180330_0942.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.4 on 2018-03-30 09:42
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is not true anymore, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About? Version of Django?
When you create a custom migration, it's easier to create an empty one and edit it.

Copy link
Owner

Choose a reason for hiding this comment

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

The biggest part of the file is not django generated anymore so you can drop the comment saying it's django code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, right but who reads comments nowaday?! :-°

from __future__ import unicode_literals

from django.db import migrations


def migrate_help_links(apps, schema_editor):
Cards = apps.get_model("cards", "Card")
for card in Cards.objects.all():
new_value = card.help_links
if '\n' in new_value:
Copy link
Owner

Choose a reason for hiding this comment

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

new_values.splitlines() should work. No?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't know about this method and you are right!

new_value = new_value.split('\n')
else:
new_value = list(new_value)
card.help_links = str(new_value)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand why you cast it back to str.

card.save()


class Migration(migrations.Migration):

dependencies = [
('cards', '0009_card_published'),
]

operations = [
migrations.RunPython(migrate_help_links),
]
32 changes: 17 additions & 15 deletions src/cards/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
from django.db import models
from django.utils.translation import ugettext_lazy as _

from base.custom_models import ListField

DATA_SOURCE_STATUS = (
('VERIFIED', 'Verified'),
('UNVERIFIED', 'Unverified')
Expand All @@ -19,27 +21,27 @@ def get_card_image_path(instance, filename):


class DataSource(models.Model):
name = models.CharField(max_length=256)
link = models.TextField(max_length=4096)
name = models.CharField(max_length=256, blank=True, null=True)
link = models.TextField(max_length=4096, blank=True, null=True)
status = models.CharField(max_length=16,
choices=DATA_SOURCE_STATUS,
default='UNVERIFIED')
default='UNVERIFIED', blank=True, null=True)

def __str__(self):
return "<DataSource {name}:{status}>".format(name=self.name, status=self.status)
return "<DataSource {name}:{status}>".format(name=self.name,
status=self.status)


class CardStat(models.Model):
waste_reduction = models.DecimalField(decimal_places=1,
max_digits=6)
co2_reduction = models.DecimalField(decimal_places=1,
max_digits=6)
water_use_reduction = models.DecimalField(decimal_places=1,
max_digits=6)
status = models.CharField(max_length=16,
choices=STATS_STATUS,
default='ACTIVE')
year = models.SmallIntegerField()
waste_reduction = models.DecimalField(decimal_places=1, max_digits=6,
blank=True, null=True)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you align on opened parenthesis?

co2_reduction = models.DecimalField(decimal_places=1, max_digits=6,
blank=True, null=True)
water_use_reduction = models.DecimalField(decimal_places=1, max_digits=6,
blank=True, null=True)
status = models.CharField(max_length=16, choices=STATS_STATUS,
default='ACTIVE', blank=True, null=True)
year = models.SmallIntegerField(blank=True, null=True)
data_sources = models.ManyToManyField(DataSource)


Expand All @@ -59,7 +61,7 @@ class Card(models.Model):
cost_score = models.SmallIntegerField()

# External links
help_links = models.TextField(blank=True, null=True)
help_links = ListField(blank=True, null=True)

published = models.BooleanField(_('published'), default=False)

Expand Down
15 changes: 10 additions & 5 deletions src/cards/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class Meta:
class CardSerializer(serializers.ModelSerializer):
card_stats = CardStatSerializer()
slug = serializers.CharField(required=False)
help_links = serializers.ListField(
Copy link
Owner

Choose a reason for hiding this comment

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

The doc specify that that you should not invoke it directly but use serializers.CharField(many=True) instead.
Any reason you didn't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm I'd like to know about the doc saying that?
I just found that: http://www.django-rest-framework.org/api-guide/fields/#listfield
But I didn't find what you are talking about?

Copy link
Owner

Choose a reason for hiding this comment

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

child=serializers.CharField(),
required=False
)

class Meta:
model = Card
Expand All @@ -39,8 +43,8 @@ class Meta:
'cost_score', 'card_stats', 'help_links')

def create(self, data):
data_stats = data.pop("card_stats")
data_sources = data_stats.pop("data_sources")
data_stats = data.pop("card_stats", {})
data_sources = data_stats.pop("data_sources", [])
sources = self.get_sources(data_sources)
stats = CardStat.objects.create(**data_stats)
stats.data_sources.add(*sources)
Expand All @@ -50,8 +54,8 @@ def create(self, data):
return card

def update(self, instance, validated_data):
data_stats = validated_data.pop("card_stats")
data_sources = data_stats.pop("data_sources")
data_stats = validated_data.pop("card_stats", {})
data_sources = data_stats.pop("data_sources", [])
sources = self.get_sources(data_sources)
stats = instance.card_stats

Expand All @@ -73,7 +77,8 @@ def get_sources(self, source_list):
sources = []
for data_src in source_list:
try:
ds = DataSource.objects.get(link=data_src['link'])
ds = DataSource.objects.get(name=data_src['name'],
link=data_src['link'], status=data_src['status'])
Copy link
Owner

Choose a reason for hiding this comment

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

**data_src?

except DataSource.DoesNotExist:
ds = DataSource.objects.create(**data_src)
sources.append(ds)
Expand Down
50 changes: 36 additions & 14 deletions src/static/containers/AdminCardAdd.jsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { cloneDeep } from 'lodash';
import React, { Component } from 'react';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import { Col, Form, Layout, Spin } from 'antd';
import { push } from 'react-router-redux';
import PropTypes from 'prop-types';
import { Col, Form, Layout, Spin } from 'antd';
import { get, cloneDeep } from 'lodash';

import AdminMenu from './AdminMenu';
import * as actionCreators from '../actions/cards';
Expand Down Expand Up @@ -36,7 +37,16 @@ class AdminCardAdd extends Component {
}
};

createCard = (e) => {
cancelCard = () => {
const {
form: { resetFields },
dispatch
Copy link
Owner

Choose a reason for hiding this comment

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

trailing comma

} = this.props;
resetFields();
dispatch(push('/zw-admin/card'));
};

validateCard = (e) => {
e.preventDefault();
const {
token,
Expand All @@ -48,11 +58,6 @@ class AdminCardAdd extends Component {
return;
}
const clonedValues = cloneDeep(values);
if (clonedValues.help_links) {
clonedValues.help_links = clonedValues.help_links.join('\n');
} else {
clonedValues.help_links = '';
}
if (this.state.editing) {
const { slug } = this.props.match.params;
editCard(token, slug, clonedValues);
Expand All @@ -67,15 +72,29 @@ class AdminCardAdd extends Component {
}

render() {
const { isFetchingCard, form } = this.props;
const { isFetchingCard, form, cardData } = this.props;
let cardForm;
// Update when we link the fetch!
const initialData = {};
let initialData = {};
if (this.state.editing) {
cardForm = generateForm(form, this.createCard, cardFields, initialData, 'Edit card!');
} else {
cardForm = generateForm(form, this.createCard, cardFields, initialData, 'Create card!');
initialData = {
Copy link
Owner

Choose a reason for hiding this comment

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

How about using react-form and initial values, thus we might need this: https://github.com/zhdmitry/redux-form-antd? And https://stackoverflow.com/a/45495643/1450754 for component not available in redux-form-antd.
Yeah I know that means a big refact 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe for a future enhancement, since it's not really in the scope of this issue and not necessary for it to work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about using another lib which gives us not so much:
Some custom components we can create easily and the initial values will stay the same.
I considered passing the data directly to the generateForm in order to hide a little bit the split in initialData but...

Copy link
Owner

Choose a reason for hiding this comment

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

To be debated elsewhere :)
But I agree for now, let's get it working like that.

title: get(cardData, 'title', ''),
description: get(cardData, 'description', ''),
waste_reduction_score: get(cardData, 'waste_reduction_score'),
difficulty_score: get(cardData, 'difficulty_score'),
cost_score: get(cardData, 'cost_score'),
published: get(cardData, 'published', false),
help_links: get(cardData, 'help_links', []),
card_stats: {
co2_reduction: get(cardData, 'card_stats.co2_reduction'),
waste_reduction: get(cardData, 'card_stats.waste_reduction'),
water_use_reduction: get(cardData, 'card_stats.water_use_reduction'),
year: get(cardData, 'card_stats.year'),
status: get(cardData, 'card_stats.status'),
data_sources: get(cardData, 'card_stats.data_sources', []),
},
};
}
cardForm = generateForm(form, this.validateCard, this.cancelCard, cardFields, initialData, this.state.editing ? 'Edit card!' : 'Create card!');
return (
<Layout>
<Sider>
Expand Down Expand Up @@ -103,6 +122,7 @@ AdminCardAdd.defaultProps = {
};

AdminCardAdd.propTypes = {
dispatch: PropTypes.func.isRequired,
token: PropTypes.string.isRequired,
isFetchingCard: PropTypes.bool.isRequired,
form: PropTypes.shape().isRequired,
Expand All @@ -116,11 +136,13 @@ AdminCardAdd.propTypes = {
editCard: PropTypes.func.isRequired,
cardFetch: PropTypes.func.isRequired,
}).isRequired,
cardData: PropTypes.shape({}),
};

const mapStateToProps = state => ({
token: state.auth.token,
isFetchingCard: state.cards.isFetchingCard,
cardData: state.cards.current_card,
});

const mapDispatchToProps = dispatch => ({
Expand Down
43 changes: 28 additions & 15 deletions src/static/containers/AdminUserAdd.jsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import React, { Component } from 'react';
import { get } from 'lodash';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import { push } from 'react-router-redux';
import PropTypes from 'prop-types';
import { get } from 'lodash';
import { Col, Form, Layout } from 'antd';

import AdminMenu from './AdminMenu';
Expand All @@ -29,11 +30,20 @@ class AdminUserAdd extends Component {
};

componentWillReceiveProps = (nextProps) => {
if (nextProps.user_data) {
if (nextProps.match.params.username) {
this.setState({ editing: true });
}
};

cancelUser = () => {
const {
form: { resetFields },
dispatch
Copy link
Owner

Choose a reason for hiding this comment

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

trailing comma

} = this.props;
this.props.form.resetFields();
dispatch(push('/zw-admin/user'));
};

createUser = (e) => {
e.preventDefault();
const {
Expand All @@ -57,21 +67,23 @@ class AdminUserAdd extends Component {
isFetchingUser,
form,
} = this.props;
const initialData = {
username: get(this.props, 'user_data.username', ''),
email: get(this.props, 'user_data.email', ''),
is_staff: get(this.props, 'user_data.is_staff', false),
has_garden: get(this.props, 'user_data.has_garden', false),
home_owner: get(this.props, 'user_data.home_owner', false),
do_smoke: get(this.props, 'user_data.do_smoke', false),
gender: get(this.props, 'user_data.gender', 'M'),
};
let userForm;
let initialData = {};
let userFields = createUserFields;
Copy link
Owner

Choose a reason for hiding this comment

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

not really accurate name. userFieldsAction?

let btnText = 'Create user!';
Copy link
Owner

Choose a reason for hiding this comment

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

Translate to french.

if (this.state.editing) {
userForm = generateForm(form, this.createUser, editUserFields, initialData, 'Edit User!');
} else {
userForm = generateForm(form, this.createUser, createUserFields, initialData, 'Create User!');
initialData = {
username: get(this.props, 'user_data.username', ''),
email: get(this.props, 'user_data.email', ''),
is_staff: get(this.props, 'user_data.is_staff', false),
has_garden: get(this.props, 'user_data.has_garden', false),
home_owner: get(this.props, 'user_data.home_owner', false),
do_smoke: get(this.props, 'user_data.do_smoke', false),
gender: get(this.props, 'user_data.gender'),
};
userFields = editUserFields;
btnText = 'Edit User!';
Copy link
Owner

Choose a reason for hiding this comment

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

Translate to french.

}
const userForm = generateForm(form, this.createUser, this.cancelUser, userFields, initialData, btnText);

return (
<Layout>
Expand All @@ -98,6 +110,7 @@ AdminUserAdd.defaultProps = {
};

AdminUserAdd.propTypes = {
dispatch: PropTypes.func.isRequired,
token: PropTypes.string.isRequired,
isFetchingUser: PropTypes.bool.isRequired,
form: PropTypes.shape().isRequired,
Expand Down
20 changes: 11 additions & 9 deletions src/static/containers/ZeroCard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -216,16 +216,18 @@ ZeroCard.propTypes = {
waste_reduction_score: PropTypes.number.isRequired,
difficulty_score: PropTypes.number.isRequired,
cost_score: PropTypes.number.isRequired,
help_links: PropTypes.string,
help_links: PropTypes.arrayOf(PropTypes.string),
card_stats: PropTypes.shape({
co2_reduction: PropTypes.number,
waste_reduction: PropTypes.number,
water_use_reduction: PropTypes.number,
data_sources: PropTypes.shape({
name: PropTypes.string,
link: PropTypes.string,
status: PropTypes.string,
}),
co2_reduction: PropTypes.string,
Copy link
Owner

Choose a reason for hiding this comment

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

Why string? We should send numbers here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

encode/django-rest-framework#508

Changed the settings of REST_FRAMEWORK not to send string instead of decimal

waste_reduction: PropTypes.string,
water_use_reduction: PropTypes.string,
data_sources: PropTypes.arrayOf(
PropTypes.shape({
name: PropTypes.string,
link: PropTypes.string,
status: PropTypes.string,
})
),
}),
}).isRequired,
token: PropTypes.string.isRequired,
Expand Down
3 changes: 3 additions & 0 deletions src/static/utils/Form.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.validate-form {
margin-right: 10px;
}
2 changes: 1 addition & 1 deletion src/static/utils/forms/components/helpLink.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class HelpLink extends Component {
constructor(props) {
super(props);
this.state = {
helplinks: defaultTo(props.values, []),
helpLinks: defaultTo(props.value, []),
};
};

Expand Down
2 changes: 1 addition & 1 deletion src/static/utils/forms/components/sources.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class Sources extends Component {
constructor(props) {
super(props);
this.state = {
sources: defaultTo(props.values, []),
sources: defaultTo(props.value, []),
};
};

Expand Down
1 change: 1 addition & 0 deletions src/static/utils/forms/user.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import { Icon, Input, Select, Switch } from 'antd';

const Option = Select.Option;

const usernameField = {
label: 'Username',
Expand Down
Loading