Multiple Select for Teammembers

Needs retest after #684016: Fix E_NOTICE notifications is fixed.

stormteam.multiple_select.patch queued for re-testing.

RTBC from a read through, but would like someone to test before marking so please.

Two things from testing:
- The select box is very tall. Would it be possible to make it smaller please? Perhaps 10 lines are visible (or better fewer).
- The first entry is now even less intuitive. We should rename it '- none -' perhaps rather than '-'.

Should be a great improvement though.

Marking CNR for testbot.

Minor point - label now can be plural.

More major point - team members don't seem to be saved correctly. I just tried creating a team - it remembers which team members are selected (because when edit is clicked they are still there), but on node view do not display.

On editing and saving an existing team, same thing happens.

CNR for testbot.

PHP errors on viewing existing teams:

# Warning: Invalid argument supplied for foreach() in theme_stormteam_view() (line 36 of /[root]/sites/all/modules/storm/stormteam/stormteam.theme.inc).
# Warning: implode() [<a href='function.implode'>function.implode</a>]: Invalid arguments passed in theme_stormteam_view() (line 62 of /[root]/sites/all/modules/storm/stormteam/stormteam.theme.inc).

Perhaps a database update to the new format would be better than supporting both formats?

Hi Carsten,

I left this for a while in case anybody else had views on it - but I think it would be better to update the data straight off - if you have concerns about scalability, then perhaps it could be done using the batch api. The reason is that a number of people do not upgrade to every release, so I think it is important to ensure the change is done cleanly - rather than leaving the possibility of having half of the data stored in each format.

Hope thats ok,
Julian

Two points from my readthrough:
- Minor spelling errors in doxygen comment - Implementation and Update
- We shouldn't use the access_sql() functions in db updates as we want to update all records

I haven't been able to test yet.

I've downloaded this patch and installed it and ran the DB updates. It looks OK so far.

I'm getting the following error:

Parse error: syntax error, unexpected T_ARRAY in /home2/pmtbsmit/public_html/drupal/sites/all/modules/storm/stormteam/stormteam.module on line 280

I've checked my patch 3 times.

A better way of posting this amount of code would be in an attached text file, or via a link to a pastebin. It is hard to help with it in this format.

Bear in mind that the patch may need reworking to account for changes in Storm since it was created.

Also, I'm quite keen to get something like this in once I'm happy with the patch.

Sorry, here's the txt file. Any way to attach your patched files?

Thx

There is a typo on line 280 of your file.

It should be $options = $options + ...

It allows me to select multiple persons for a team, now, but when I try to assign a task, the pull down only shows the project manager, and the team, not individuals on the team.

@Carsten Müller, would you have a moment to revisit this patch at all?

I'm not smart enough to figure out how to implement the patch programatically, so I input changes by hand. Sucky way to do it.

Thanks,

Wes

Thanks, needs retesting on the new 2.x, but it shouldn't change anything.

@Wes Ashworth, see http://drupal.org/patch/apply .

#29: stormteam.multiple_select.patch queued for re-testing.

Minor changes needed from a read through of the patch.

I'd like to test this properly on an install too, which I'll try to do soon so that this can go in if poss.

+++ stormteam/stormteam.install	(working copy)
@@ -87,3 +87,52 @@
+ * Uddate for Multiple Select for Teammembers

Typo - Uddate / Update

Also, there is trailing whitespace on some lines which should be removed. Dreditor can spot this very easily.

Powered by Dreditor.

I tried to reroll this patch for the testbot, but got an error on patching - malformed patch.

Tried #35 and #37.

The 2.x branch was opened for 2 reasons:
- To allow continued development whilst giving a more stable branch for those who are happy with the current level of features
- To allow for more disruptive features such as a mandatory dependency on views

See #1011492: Storm 6.x-2.x branch

Would it be possible to have a quick conversation with minorOffense on the data structures here please? (See #1052154: Storm Team module schema). I'd just like to make sure the changes you're both suggesting are on the same page, so that we don't end up changing things down the road.

A couple of points from within the code too, but otherwise I think we're there...

+++ stormteam/stormteam.install	(working copy)
@@ -87,3 +87,51 @@
+function stormteam_update_6102() {

This needs to start 62xx as the patch will go into the 2.x branch.

+++ stormteam/stormteam.module	(working copy)
@@ -247,13 +247,13 @@
   $breadcrumb = array(
-    l('Storm', 'storm'),
-    l('Teams', 'storm/teams'),
+  l('Storm', 'storm'),
+  l('Teams', 'storm/teams'),
   );

There are a couple of moments like this - where the indentation has been changed on areas not affected by the patch. To me, this new structure is less clear. Is there a reason why this change was made?

Powered by Dreditor.

I've opened #1081082: Git clone fails during patch testing in the infrastructure queue as this seems to be a problem with the testbot.

#46: stormteam-multiple-848444-7-git.patch queued for re-testing.

Setting to needs review to force another test

Great now that the Testbot is ok-ing the patches again!

Carsten, is it worth having the admin switch between a multiple select and the existing? Surely this is a great usability improvement that we could assume most people would want.

We might be able to simplify by moving entirely to the new system?

subscribe

Sorry for not responding. It sounds like there are good reasons for providing the options.

I haven't marked this as RTBC or committed yet because I haven't had time to test, but I have no further questions about this, feel free to commit if you feel that it is ready.

Feel free to commit things if you feel they are ready - no need to wait for me if I'm holding things up.

I've just successfully tested and pushed this into Git.

Sorry that I haven't had a chance to look sooner.

I spoke too soon. Continued with the patch applied and on viewing the team node, got the word "Array" instead of a list of teammembers.

Reverted patch for now until this can be fixed.

No problem - that's how it is for everyone.

I saw that this patch didn't apply against the current 6.x-2.x-dev without breaking, so I rewrote it. I don't see an array with the list of team members, but I'm too new to Storm to know what else it actually breaks. YMMV.

Wait, that was really dumb of me, I only saw the first page of Carsten's patch. Amazingly, it did let me define teams of arbitrary size... However, the scope of the problem is larger than I'd originally thought... I'm going to have to pass on this one unless I actually commit to Storm for project management.

I am currently working on a Project using Storm. For stormteam I could imagine the following adjustment (images). If we get some feedback we could provide a patch.

I made a multiple select for the teammembers.

Patch is in the attachment.

I had to edit some of the .test-files because i changed the form.
I ran it through simpletest, it should work properly now.

These exceptions weren't displayed in my simpletest... strange.
Now it should really pass the tests. ;)

Oops... I fixed this bug too fast and didn't test it properly, sorry.
I just introduced another bug... but it's fixed now.

I tested patch in comment 71. This is an awesome new feature. Easier to add or remove team members. Thanks all :)

There is an issue with the display of "Team Names" though. Steps to reproduce:
1. Go to /node/add/stormteam
2. Create a new team. Add two team members. Click on "Save" button.
3. Issue is on the next page at /storm/teams. Under "Team Name" column. The "Team Name" is display twice. They both link to the same node. Find attached screenshots to clarify. Expected result is the "Team Name" should be display only once.

If you redo above steps but add three team members the Team Name is display three times. And so on.

Using
* Fresh Drupal 6.27
* Storm 2.x-1.x-dev 2012-Oct-20

Thanks for testing.

Considering that we've started with the port to Drupal 7 now, we need to have a patch for D7 before D6.

Sorry for the inconvenience, but I'm sure you'll agree this is best practice for the longer term.

Same thing, make sense to me :)

Any volunteer to port this patch? I would be happy to contribute testing.