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

#76 : edit forms #83

merged 9 commits into from
Apr 3, 2018

Conversation

Far3t
Copy link
Collaborator

@Far3t Far3t commented Mar 30, 2018

Be careful with the custom migration.
Worked with me but I do not know if there was another type before a plain text separated by \n for help_links...

@Far3t Far3t requested a review from blatinier March 30, 2018 14:33
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!

@@ -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?! :-°

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.

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?

@@ -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.

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.

let userForm;
let initialData = {};
let userFields = createUserFields;
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.

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

if (splitArray[0] in initialData) {
let init = initialData[splitArray[0]];
for (let data of splitArray.slice(1)) {
init = init[data];
Copy link
Owner

Choose a reason for hiding this comment

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

This piece seems either buggy either too much complicate for what it aims to do… What's the purpose of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Getting the initialData from the object we get:
{ card_stats: { waste_reduction: 1, ... }, }
with the id field like "card_stats.waste_reduction"

Copy link
Owner

Choose a reason for hiding this comment

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

Could get from lodash do the job? It tipically can do this: get(object, "card_stats.waste_reduction", defaultValue)

See: https://lodash.com/docs/4.17.5#get

onClick={onCancel}
size="large"
>
Cancel
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

new_value = list(new_value)
card.help_links = str(new_value)
new_value = card.help_links.splitlines()
card.help_links = 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.

The use of new_value is not necessary anymore

@blatinier blatinier merged commit 95b935b into devel Apr 3, 2018
@blatinier blatinier deleted the issue76/edit_forms branch April 3, 2018 08:43
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.

2 participants