Bug/error undefined add group

[mnmercer]

Addresses issue #214 by adding default value for leaders on new groups. Also fixes a React console error about unique keys in GroupRow.

[jeffklassen]

That might make sense, but the web app doesn't always pull from the server. Such as when a new group is added. Were you seeing it as working?

On Aug 15, 2017 14:40, "mnmercer" notifications@github.com wrote:

@mnmercer commented on this pull request.

In client/src/App/components/groups/createEdit/GroupCreateEditSurface.jsx https://github.com/MosaicGroups/mosaic-groups/pull/216#discussion_r133179407 :

@@ -36,6 +36,9 @@ class GroupCreateEditSurface extends React.Component { // topics get passed from the form as a single string value, // but must be passed to the server as an array group.topics = [group.topics];

• if (!group.leaders) {
• group.leaders = [];

They do default to that user, but it happens in the server code. Lines 60-62 in groupsController. Figured since the logic for that was there, we need only default it to an empty array here.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MosaicGroups/mosaic-groups/pull/216#discussion_r133179407, or mute the thread https://github.com/notifications/unsubscribe-auth/AHCOwIql9NHWY8CNXCC-R4v312Iae31iks5sYZHUgaJpZM4O3IhJ .

[mnmercer]

I see what you mean. It's the behavior I noticed where we just push the group into the list when added instead of pulling it from the server. In that case, could we just do the logic in the webapp rather than the server? I have a thing against code duplication...

[jeffklassen]

It's a balance between code deduplication and enforcing business logic on the server.

On Aug 15, 2017 18:28, "mnmercer" notifications@github.com wrote:

I see what you mean. It's the behavior I noticed where we just push the group into the list when added instead of pulling it from the server. In that case, could we just do the logic in the webapp rather than the server? I have a thing against code duplication...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MosaicGroups/mosaic-groups/pull/216#issuecomment-322517423, or mute the thread https://github.com/notifications/unsubscribe-auth/AHCOwCeUk8aq7Adcrz9gJ2NGFjoYA9Alks5sYcc1gaJpZM4O3IhJ .

[mnmercer]

It seems like we would be increasing the amount of code maintenance needed if we ever need to update that condition though. As a side note, I also don't understand why we are adding the group to the groups array in the webapp before we've successfully added it to the database on the server.

[jeffklassen]

All great questions and I'd be super happy to discuss, but given that this is the way the larger application works (in more than just the groups section of the site), perhaps we can move the conversation from this PR into its own issue?

For now, are there any additional questions or suggestions pertaining to this fix for the problem?

On Aug 16, 2017 04:23, "mnmercer" notifications@github.com wrote:

It seems like we would be increasing the amount of code maintenance needed if we ever need to update that condition though. As a side note, I also don't understand why we are adding the group to the groups array in the webapp before we've successfully added it to the database on the server.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MosaicGroups/mosaic-groups/pull/216#issuecomment-322646299, or mute the thread https://github.com/notifications/unsubscribe-auth/AHCOwKmFx6bVC6hCzUi3PMPCE18A7PmDks5sYlKVgaJpZM4O3IhJ .

[mnmercer]

Well both our branches fix bugs other than the error Amanda mentioned so we really need all that included in dev. We can go with your fix in the end for now, but I would like to talk about cleaning up some of the logic at some point.

[mnmercer]

Was the fix for the GroupRow from this branch included in #217 ?