Permalink
Browse files

[FIXED JENKINS-13697]: retrieve notificationOnly value via JSON repre…

…sentation
  • Loading branch information...
syl20bnr committed Dec 11, 2012
1 parent 8767488 commit ffbda2c14adc98c995674ea7225518edd5390375
Showing with 40 additions and 4 deletions.
  1. +40 −4 src/main/java/hudson/plugins/ircbot/IrcPublisher.java
@@ -33,7 +33,9 @@
import java.util.SortedMap;
import java.util.logging.Logger;

import net.sf.json.JSONArray;
import net.sf.json.JSONObject;
import net.sf.json.util.JSONUtils;

This comment has been minimized.

@syl20bnr

syl20bnr Dec 11, 2012

Author Member

import net.sf.json.util.JSONUtils; not needed ?

This comment has been minimized.

@kutzi

kutzi Dec 11, 2012

Member

Are you commenting on your own commit!?
If it's not needed, why did you add it?

This comment has been minimized.

@syl20bnr

syl20bnr Dec 11, 2012

Author Member

This is silly ;-) Just I needed to go to sleep and didn't want to forget about this.
So you can make the changes or I'll do tonight (promise I will go to sleep sooner and request pulling instead of doing silly things !)


import org.kohsuke.stapler.StaplerRequest;

@@ -222,6 +224,30 @@ protected Object readResolve() {
}
}

/**
* Check boxes values are not passed in the posted form when they are unchecked.
* The workaround consists in acceding these values via the JSON representation.
*/
private static List<JSONObject> fillChannelsFromJSON(JSONObject root){
List<JSONObject> result = null;
JSONObject chan = root.optJSONObject("channels");
if (chan != null){
result = new ArrayList<JSONObject>();
result.add(chan);
}
else{
JSONArray chans = root.optJSONArray("channels");
if (chans != null){
result = new ArrayList<JSONObject>();
for(int i=0; i<chans.size(); ++i){
chan = chans.getJSONObject(i);
result.add(chan);
}
}
}
return result;
}

/**
* @see hudson.model.Descriptor#configure(org.kohsuke.stapler.StaplerRequest)
*/
@@ -251,10 +277,20 @@ public boolean configure(StaplerRequest req, JSONObject formData) throws FormExc

String[] channelsNames = req.getParameterValues("irc_publisher.channel.name");
String[] channelsPasswords = req.getParameterValues("irc_publisher.channel.password");
String[] notifyOnlys = req.getParameterValues("irc_publisher.chat.notificationOnly");
// only checked state can be queried, unchecked state are ignored and the size of
// notifyOnlys may be lower than the size of channelNames
// so getting the values via stapler is unreliable.
// String[] notifyOnlys = req.getParameterValues("irc_publisher.chat.notificationOnly");

List<IMMessageTarget> targets = Collections.emptyList();
if (channelsNames != null) {
// JENKINS-13697: Get the data from the JSON representation which always returns
// a value. The downside is that we are dependent on the data structure.
List<JSONObject> jchans = null;
JSONObject enabled = formData.optJSONObject("enabled");
if (enabled != null){

This comment has been minimized.

@kutzi

kutzi Dec 13, 2012

Member

I think this could be refactored to better describe what's happening here. Basically the enabled check seems to be duplicated. Once here and once further down when checking jchans != null.

I know the code was not really clean before ;-), but that should even more make us go to make it better.

This comment has been minimized.

@syl20bnr

syl20bnr Dec 14, 2012

Author Member

You've got a point here but it's channelNames and jchans which are testing the same thing (channel definition existence) or channelNames and enabled (to define a channel we must enable the bot). So inside this scope we could remove the tests on both enabled and jchans. Does it make sense ?

jchans = fillChannelsFromJSON(enabled);
}
targets = new ArrayList<IMMessageTarget>(channelsNames.length);
for (int i=0; i < channelsNames.length; i++) {

@@ -263,7 +299,7 @@ public boolean configure(StaplerRequest req, JSONObject formData) throws FormExc
}

String password = Util.fixEmpty(channelsPasswords[i]);
boolean notifyOnly = notifyOnlys != null ? "on".equalsIgnoreCase(notifyOnlys[i]) : false;
boolean notifyOnly = jchans.size() > 0 ? jchans.get(i).getBoolean("notificationOnly") : false;

This comment has been minimized.

@syl20bnr

syl20bnr Dec 11, 2012

Author Member

should be:

boolean notifyOnly = jchans != null ? jchans.get(i).getBoolean("notificationOnly") : false;


targets.add(new GroupChatIMMessageTarget(channelsNames[i], password, notifyOnly));
}
@@ -321,10 +357,10 @@ public String getHelpFile() {
public Publisher newInstance(StaplerRequest req, JSONObject formData) throws FormException {
String[] channelsNames = req.getParameterValues("irc_publisher.channel.name");
String[] channelsPasswords = req.getParameterValues("irc_publisher.channel.password");
String[] notifyOnlys = req.getParameterValues("irc_publisher.chat.notificationOnly");

List<IMMessageTarget> targets = Collections.emptyList();
if (channelsNames != null) {
List<JSONObject> jchans = fillChannelsFromJSON(formData);
targets = new ArrayList<IMMessageTarget>(channelsNames.length);
for (int i=0; i < channelsNames.length; i++) {

@@ -333,7 +369,7 @@ public Publisher newInstance(StaplerRequest req, JSONObject formData) throws For
}

String password = Util.fixEmpty(channelsPasswords[i]);
boolean notifyOnly = notifyOnlys != null ? "on".equalsIgnoreCase(notifyOnlys[i]) : false;
boolean notifyOnly = jchans.size() > 0 ? jchans.get(i).getBoolean("notificationOnly") : false;

This comment has been minimized.

@syl20bnr

syl20bnr Dec 11, 2012

Author Member

should be:

boolean notifyOnly = jchans != null ? jchans.get(i).getBoolean("notificationOnly") : false;

targets.add(new GroupChatIMMessageTarget(channelsNames[i], password, notifyOnly));
}
}

4 comments on commit ffbda2c

@kutzi

This comment has been minimized.

Copy link
Member

kutzi replied Dec 11, 2012

I'm really thankful that you looked at this issue, but I'd be grateful if you'd open pull requests instead of commiting directly.

@syl20bnr

This comment has been minimized.

Copy link
Member Author

syl20bnr replied Dec 11, 2012

You're welcome and my apologies for this. It won't happen again!

@syl20bnr

This comment has been minimized.

Copy link
Member Author

syl20bnr replied Dec 13, 2012

I'm not sure about what you meant, do you prefere to rollback this fix and want me to submit a pull request instead ? No problem for me to do it.

@kutzi

This comment has been minimized.

Copy link
Member

kutzi replied Dec 13, 2012

No prob

Please sign in to comment.