Skip to content
Permalink
Browse files

[FIXED JENKINS-18680] Update members of a ListView using a regular li…

…stener, not View.onJobRenamed which does not work with folders.
  • Loading branch information...
jglick committed Dec 19, 2013
1 parent a11df3f commit 6d3c2e0d849dd76b6507daca1cc910f75418e941
@@ -81,11 +81,6 @@ public String getPostConstructLandingPage() {
return ""; // there's no configuration page
}

@Override
public void onJobRenamed(Item item, String oldName, String newName) {
// noop
}

@Override
protected void submit(StaplerRequest req) throws IOException, ServletException, FormException {
// noop
@@ -83,7 +83,6 @@

/**
* Internal method. Called by {@link Item}s when they are deleted by users.
* Should normally call {@link ItemListener#fireOnDeleted} and {@link View#onJobRenamed} (with a null {@code newName}).
*/
void onDeleted(T item) throws IOException;
}
@@ -28,6 +28,7 @@
import hudson.Util;
import hudson.diagnosis.OldDataMonitor;
import hudson.model.Descriptor.FormException;
import hudson.model.listeners.ItemListener;
import hudson.util.CaseInsensitiveComparator;
import hudson.util.DescribableList;
import hudson.util.FormValidation;
@@ -37,13 +38,18 @@

import java.io.IOException;
import java.util.*;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;

import javax.annotation.concurrent.GuardedBy;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;

import net.sf.json.JSONObject;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.HttpResponse;
@@ -302,12 +308,6 @@ public HttpResponse doRemoveJobFromView(@QueryParameter String name) throws IOEx
return HttpResponses.ok();
}

@Override
public synchronized void onJobRenamed(Item item, String oldName, String newName) {
if(jobNames.remove(oldName) && newName!=null)
jobNames.add(newName);
}

/**
* Handles the configuration submission.
*
@@ -387,4 +387,56 @@ public FormValidation doCheckIncludeRegex( @QueryParameter String value ) throws
public static List<ListViewColumn> getDefaultColumns() {
return ListViewColumn.createDefaultInitialColumnList();
}

@Restricted(NoExternalUse.class)
@Extension public static final class Listener extends ItemListener {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Apr 24, 2014

Member

From reading the code this seems to miss an ACL.impersonate, otherwise it won't get everything in case the actions are initiated by a low-privilege user (missing View.READ).

This comment has been minimized.

Copy link
@jglick

jglick Apr 25, 2014

Author Member

Or Job.READ for that matter. Probably true; if so, should be easily caught by a functional test. Please make sure this is filed (JIRA or PR, according to how much time you have).

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Apr 25, 2014

Member

Chances are you can Job.READ all ancestors of the item you renamed. And list views cannot usually contain items outside their ViewGroup/ItemGroup.

Filed for now as JENKINS-22769.

@Override public void onLocationChanged(Item item, String oldFullName, String newFullName) {
for (Item g : Jenkins.getInstance().getAllItems()) {
if (g instanceof ViewGroup) {
ViewGroup vg = (ViewGroup) g;
for (View v : vg.getViews()) {
if (v instanceof ListView) {
ListView lv = (ListView) v;
synchronized (lv) {
Set<String> oldJobNames = new HashSet<String>(lv.jobNames);
lv.jobNames.clear();
for (String oldName : oldJobNames) {
lv.jobNames.add(Items.computeRelativeNamesAfterRenaming(oldFullName, newFullName, oldName, vg.getItemGroup()));
}
if (!oldJobNames.equals(lv.jobNames)) {
try {
g.save();

This comment has been minimized.

Copy link
@olivergondza

olivergondza Jul 20, 2014

Member

This seems wrong. It is the view what was modified. Fixing in #1323.

} catch (IOException x) {
Logger.getLogger(ListView.class.getName()).log(Level.WARNING, null, x);
}
}
}
}
}
}
}
}
@Override public void onDeleted(Item item) {
for (Item g : Jenkins.getInstance().getAllItems()) {
if (g instanceof ViewGroup) {
ViewGroup vg = (ViewGroup) g;
for (View v : vg.getViews()) {
if (v instanceof ListView) {
ListView lv = (ListView) v;
synchronized (lv) {
if (lv.jobNames.remove(item.getRelativeNameFrom(vg.getItemGroup()))) {
try {
g.save();
} catch (IOException x) {
Logger.getLogger(ListView.class.getName()).log(Level.WARNING, null, x);
}
}
}
}
}
}
}
}
}

}
@@ -86,11 +86,6 @@ public String getPostConstructLandingPage() {
return ""; // there's no configuration page
}

@Override
public void onJobRenamed(Item item, String oldName, String newName) {
// noop
}

@Override
protected void submit(StaplerRequest req) throws IOException, ServletException, FormException {
// noop
@@ -89,13 +89,6 @@ public boolean contains(TopLevelItem item) {
return getProxiedView().contains(item);
}

@Override
public void onJobRenamed(Item item, String oldName, String newName) {
if (oldName.equals(proxiedViewName)) {
proxiedViewName = newName;
}
}

@Override
protected void submit(StaplerRequest req) throws IOException, ServletException, FormException {
String proxiedViewName = req.getSubmittedForm().getString("proxiedViewName");
@@ -119,14 +119,7 @@ public TopLevelItem doCreateItem(StaplerRequest req, StaplerResponse rsp) throws
return null;
}

@Override
public synchronized void onJobRenamed(Item item, String oldName, String newName) {
if(jobNames.remove(oldName) && newName!=null)
jobNames.add(newName);
// forward to children
for (View v : views)
v.onJobRenamed(item,oldName,newName);
}
// TODO listen for changes that might affect jobNames

protected void submit(StaplerRequest req) throws IOException, ServletException, FormException {
}
@@ -581,21 +581,9 @@ public boolean hasPermission(Permission p) {
return getACL().hasPermission(p);
}

/**
* Called when a job name is changed or deleted.
*
* <p>
* If this view contains this job, it should update the view membership so that
* the renamed job will remain in the view, and the deleted job is removed.
*
* @param item
* The item whose name is being changed.
* @param oldName
* Old name of the item. Always non-null.
* @param newName
* New name of the item, if the item is renamed. Or null, if the item is removed.
*/
public abstract void onJobRenamed(Item item, String oldName, String newName);
/** @deprecated Does not work properly with moved jobs. Use {@link ItemListener#onLocationChanged} instead. */
@Deprecated
public void onJobRenamed(Item item, String oldName, String newName) {}

@ExportedBean(defaultVisibility=2)
public static final class UserInfo implements Comparable<UserInfo> {
@@ -2434,6 +2434,7 @@ public void onRenamed(TopLevelItem job, String oldName, String newName) throws I
items.remove(oldName);
items.put(newName,job);

// For compatibility with old views:
for (View v : views)
v.onJobRenamed(job, oldName, newName);
save();
@@ -2446,6 +2447,7 @@ public void onDeleted(TopLevelItem item) throws IOException {
ItemListener.fireOnDeleted(item);

items.remove(item.getName());
// For compatibility with old views:
for (View v : views)
v.onJobRenamed(item, item.getName(), null);
save();
@@ -177,10 +177,6 @@ public boolean contains(TopLevelItem item) {
return false;
}

@Override
public void onJobRenamed(Item item, String oldName, String newName) {
}

@Override
protected void submit(StaplerRequest req) throws IOException, ServletException, FormException {
}
@@ -181,9 +181,6 @@ private ViewGroupMixIn vgmixin() {
@Override public void onRenamed(TopLevelItem item, String oldName, String newName) throws IOException {
items.remove(oldName);
items.put(newName, item);
for (View v : views) {
v.onJobRenamed(item, oldName, newName);
}
}

@Override public void renameTo(String newName) throws IOException {
@@ -193,9 +190,6 @@ private ViewGroupMixIn vgmixin() {
@Override public void onDeleted(TopLevelItem item) throws IOException {
ItemListener.fireOnDeleted(item);
items.remove(item.getName());
for (View v : views) {
v.onJobRenamed(item, item.getName(), null);
}
}

@Override public boolean canAdd(TopLevelItem item) {
@@ -24,11 +24,17 @@

package hudson.model;

import static org.junit.Assert.*;
import com.gargoylesoftware.htmlunit.html.HtmlAnchor;
import com.gargoylesoftware.htmlunit.html.HtmlPage;
import hudson.Functions;

import hudson.matrix.AxisList;
import hudson.matrix.MatrixProject;
import hudson.matrix.TextAxis;
import java.io.IOException;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Bug;
@@ -38,13 +44,6 @@
import org.jvnet.hudson.test.recipes.LocalData;
import org.xml.sax.SAXException;

import com.gargoylesoftware.htmlunit.html.HtmlAnchor;
import com.gargoylesoftware.htmlunit.html.HtmlPage;
import hudson.matrix.AxisList;
import hudson.matrix.MatrixProject;
import hudson.matrix.TextAxis;
import java.util.Collections;

public class ListViewTest {

@Rule public JenkinsRule j = new JenkinsRule();
@@ -112,4 +111,28 @@ private void checkLinkFromItemExistsAndIsValid(Item item, ItemGroup ig, Item top
assertEquals(Collections.singletonList(mp), v.getItems());
}

@Bug(18680)
@Test public void renamesMovesAndDeletes() throws Exception {
MockFolder top = j.createFolder("top");
MockFolder sub = top.createProject(MockFolder.class, "sub");
FreeStyleProject p1 = top.createProject(FreeStyleProject.class, "p1");
FreeStyleProject p2 = sub.createProject(FreeStyleProject.class, "p2");
FreeStyleProject p3 = top.createProject(FreeStyleProject.class, "p3");
ListView v = new ListView("v");
top.addView(v);
v.add(p1);
v.add(p2);
v.add(p3);
assertEquals(new HashSet<TopLevelItem>(Arrays.asList(p1, p2, p3)), new HashSet<TopLevelItem>(v.getItems()));
sub.renameTo("lower");
MockFolder stuff = top.createProject(MockFolder.class, "stuff");
Items.move(p1, stuff);
p3.delete();
Thread.sleep(500); // TODO working around crappy JENKINS-19446 fix
top.createProject(FreeStyleProject.class, "p3");
assertEquals(new HashSet<TopLevelItem>(Arrays.asList(p1, p2)), new HashSet<TopLevelItem>(v.getItems()));
top.renameTo("upper");
assertEquals(new HashSet<TopLevelItem>(Arrays.asList(p1, p2)), new HashSet<TopLevelItem>(v.getItems()));
}

}

0 comments on commit 6d3c2e0

Please sign in to comment.
You can’t perform that action at this time.