Comment 1 Mik Kersten 2006-12-08 14:44:36 EST

The problem here is that we need the ID of the task, not the key which is what we get from ILinkedTaskInfo.  We can search by key to find the corresponding task, but for now I have simply popped up message dialog prompting the user to use the Open Task dialog, and we fall back to opening with the web browser.

If you could give this a try with the JIRA task that it failed with that would be helpful.

Comment 2 Mik Kersten 2006-12-08 14:48:17 EST

I looked again and I think that ILinkedTaskInfo.getTaskId() will either need to return the unique task ID (not JIRA key), or the name of that method needs to change.  So over to you for now.

Comment 3 Eugene Kuleshov 2006-12-08 14:56:55 EST

(In reply to comment #2)
> I looked again and I think that ILinkedTaskInfo.getTaskId() will either need to
> return the unique task ID (not JIRA key), or the name of that method needs to
> change.  So over to you for now.

Task key for jira won't be available from the linked task info. Is is simply not available from the information we can get.

BTW, why can't you just show that open task/remote task dialog (with task id filled in) if taskui can't open the editor?

Comment 4 Mik Kersten 2006-12-08 15:03:25 EST

Not at this stage, but yes, we should do that.

I know that it's not available in the comment, but either it has to be looked up when the ILinkedTaskInfo is created (either in the Task List or on server), or we have to change the name of the method to getIdLabel() to be consistent with AbstractRepositoryTask.  Thoughts?

Comment 5 Eugene Kuleshov 2006-12-08 15:10:52 EST

(In reply to comment #4)
> I know that it's not available in the comment, but either it has to be looked
> up when the ILinkedTaskInfo is created (either in the Task List or on server),
> or we have to change the name of the method to getIdLabel() to be consistent
> with AbstractRepositoryTask.  Thoughts?

You know that I never liked these internal ids. Anyways, I don't think implementor of ILinkedTaskInfo should know or do anything about internal task ids. Also, changing method name is probably distractive at this stage (i.e. it will break Subclipse integration).

As a workaround you can use id returned by linked task info to query internal id, but that logic should be in open corresponding task action or some utility class that action is using.

Comment 6 Mik Kersten 2006-12-08 15:21:02 EST

OK, leaving as is for 1.0, let's revisit after.

Comment 7 Eugene Kuleshov 2006-12-09 15:00:18 EST

Hmm. Why it is assigned to me now?

Comment 8 Mik Kersten 2006-12-10 19:12:38 EST

Mistake, should not have been assigned to you.

Comment 9 Steffen Pingel 2007-01-16 12:22:29 EST

The Open Repository Task dialog should show a message that an invalid task id was entered instead of logging an error to the Eclipse log.

Comment 10 Eugene Kuleshov 2007-01-16 12:26:40 EST

I would expect it to work without any error messages. :-)

Comment 11 Steffen Pingel 2007-01-16 12:38:40 EST

Even better :).

Comment 12 Steffen Pingel 2007-01-16 12:45:04 EST

Created attachment 56973 [details]
Templates

Comment 13 Eugene Kuleshov 2007-01-16 13:28:59 EST

Comment on attachment 56973 [details]
Templates

Seems like wrong bug report.

Comment 14 Mik Kersten 2007-01-21 20:53:00 EST

*** Bug 169974 has been marked as a duplicate of this bug. ***

Comment 15 Eugene Kuleshov 2007-01-25 15:46:15 EST

Mik, are you sure you want me to work on this? You know that I always hated that handle thing and can rip it off really quick... :-)

Comment 16 Mik Kersten 2007-01-25 15:52:28 EST

No, what I was thinking is that we already have a bug for the handle thing (bug 170587) and that you have already fixed this particular bug, given the description.  If not please reopen.

Comment 17 Eugene Kuleshov 2007-01-25 15:58:18 EST

Apparently it is not fixed. Here is the new stack trace

java.lang.RuntimeException: invalid handle for task, can not contain: -, was: MNGECLIPSE-171
	at org.eclipse.mylar.tasks.core.AbstractRepositoryTask.getHandle(AbstractRepositoryTask.java:138)
	at org.eclipse.mylar.tasks.ui.TasksUiUtil.openRepositoryTask(TasksUiUtil.java:121)
	at org.eclipse.mylar.internal.tasks.ui.actions.OpenRepositoryTask.openRemoteTask(OpenRepositoryTask.java:89)
	at org.eclipse.mylar.internal.tasks.ui.actions.OpenRepositoryTask.run(OpenRepositoryTask.java:51)
	at org.eclipse.ui.internal.PluginAction.runWithEvent(PluginAction.java:253)
	at org.eclipse.ui.internal.WWinPluginAction.runWithEvent(WWinPluginAction.java:229)
	at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:545)
	at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:490)
	at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:402)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:928)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3465)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3079)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:1945)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:1909)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:425)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at org.eclipse.ui.internal.ide.IDEApplication.run(IDEApplication.java:95)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:585)
	at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethod(EclipseAppContainer.java:522)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:147)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:74)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:354)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:170)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:585)
	at org.eclipse.core.launcher.Main.invokeFramework(Main.java:339)
	at org.eclipse.core.launcher.Main.basicRun(Main.java:283)
	at org.eclipse.core.launcher.Main.run(Main.java:984)
	at org.eclipse.core.launcher.Main.main(Main.java:959)

Comment 18 Steffen Pingel 2007-01-25 20:08:36 EST

Didn't we plan to escape/encode the task id a while back so it could legally contain dashes?

Comment 19 Eugene Kuleshov 2007-01-26 00:14:40 EST

(In reply to comment #18)
> Didn't we plan to escape/encode the task id a while back so it could legally
> contain dashes?

It won't help much with JIRA because it does not use this "fancy" way to construct handle.

Comment 20 Steffen Pingel 2007-01-26 11:01:28 EST

This should be transparently handled by AbstractRepositoryTask.getHandle() and AbstractRepositoryTask.getTaskId().

Comment 21 Eugene Kuleshov 2007-01-26 11:10:52 EST

(In reply to comment #20)
> This should be transparently handled by AbstractRepositoryTask.getHandle() and
> AbstractRepositoryTask.getTaskId().

It should NOT and those methods should be removed. Not sure what is stopping Mik to fix bug 170587.

Comment 22 Steffen Pingel 2007-01-26 11:21:05 EST

I agree that fixing bug 170587 is the way to go but if that is not happing for whatever reason we should add escaping for dashes in task ids (e.g. replace single dashes by two dashes).

Comment 23 Eugene Kuleshov 2007-01-26 11:36:15 EST

(In reply to comment #22)
> I agree that fixing bug 170587 is the way to go but if that is not happing for
> whatever reason we should add escaping for dashes in task ids (e.g. replace
> single dashes by two dashes).

Again, that won't buy anything for JIRA, because its handle is not constructed like that. So, there is no point to encode.

Comment 24 Steffen Pingel 2007-01-26 14:42:45 EST

Created attachment 57619 [details]
escape task id in repository handle

The patch allows task ids that contain dashes. It fixes opening of remote task when Jira's human-friendly task id is used. It is a fairly simple non-breaking change that lifts a limitiation of the current API. I think we should consider merging until bug 170587 is resolved.

Comment 25 Steffen Pingel 2007-01-26 14:42:50 EST

Created attachment 57620 [details]
mylar/context/zip

Comment 26 Eugene Kuleshov 2007-02-02 00:06:30 EST

Mik, why it is assigned to me again?

Comment 27 Eugene Kuleshov 2007-02-14 01:14:54 EST

Mik, this is still an issue. Here is the stack trace from opening JIRA task

java.lang.RuntimeException: invalid handle for task, can not contain: -, was: MNG-2820
	at org.eclipse.mylar.internal.tasks.core.RepositoryTaskHandleUtil.getHandle(RepositoryTaskHandleUtil.java:29)
	at org.eclipse.mylar.tasks.core.TaskList.getTask(TaskList.java:401)
	at org.eclipse.mylar.internal.jira.ui.JiraTaskDataHandler.getJiraIssue(JiraTaskDataHandler.java:78)
	at org.eclipse.mylar.internal.jira.ui.JiraTaskDataHandler.getTaskData(JiraTaskDataHandler.java:58)
	at org.eclipse.mylar.tasks.ui.OpenRepositoryTaskJob.run(OpenRepositoryTaskJob.java:84)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:58)

Comment 28 Steffen Pingel 2007-02-14 01:26:32 EST

This will work once dashes in the task id part of the handle are escaped. We may need an additional fix for OpenRepositoryTaskJob though since the created handle would contain the JIRA key and not the task id. Hence it would be possible to have the same task with different handles.

Comment 29 Eugene Kuleshov 2007-02-14 01:31:31 EST

(In reply to comment #28)
> This will work once dashes in the task id part of the handle are escaped. We
> may need an additional fix for OpenRepositoryTaskJob though since the created
> handle would contain the JIRA key and not the task id. Hence it would be
> possible to have the same task with different handles.

Steffen, new method TaskList.getTask(TaskList.java:401) thet Mik just added still constructing handle and throws exception if task id contains '-'. So, this issue will be fixed if we just remove that stupid exception. 

Better fix would be to make TaskList.getTask() to search without creating handle string from passed repository url and task id.

Comment 30 Steffen Pingel 2007-02-14 01:44:47 EST

If we don't escape dashes before removing the exception we loose the ability to derive the id from the handle.

I propose a method "String AbstractRepositoryConnector.getTaskId(String key) throws CoreException" that validates the key (e.g. checks that is is numeric) and returns the id in case of JIRA.

Comment 31 Eugene Kuleshov 2007-02-14 01:52:47 EST

(In reply to comment #30)
> If we don't escape dashes before removing the exception we loose the ability to
> derive the id from the handle.

Why one would need to derive id from handle?

> I propose a method "String AbstractRepositoryConnector.getTaskId(String key)
> throws CoreException" that validates the key (e.g. checks that is is numeric)
> and returns the id in case of JIRA.

That method already gone.

Comment 32 Steffen Pingel 2007-02-14 02:04:12 EST

 (In reply to comment #31)
> > If we don't escape dashes before removing the exception we loose the ability
> to
> > derive the id from the handle.
> Why one would need to derive id from handle?

Maybe it is not needed anymore. Mik, are you planning to get rid of the remaining calls to RepositoryTaskHandleUtil.getTaskId()?

> > I propose a method "String AbstractRepositoryConnector.getTaskId(String key)
> > throws CoreException" that validates the key (e.g. checks that is is numeric)
> > and returns the id in case of JIRA.
> That method already gone.

What do you suggest to map to the JIRA task id then or do we not need a unique handle in this case at all?

Comment 33 Eugene Kuleshov 2007-02-14 02:09:03 EST

 (In reply to comment #32)
> What do you suggest to map to the JIRA task id then or do we not need a unique
> handle in this case at all?

My original idea was to allow to get task from the TaskList by repositoryUrl and the key. I guess it would need to be done on case by case basis.

Comment 34 Steffen Pingel 2007-02-14 02:22:27 EST

 (In reply to comment #33)
> (In reply to comment #32)
> > What do you suggest to map to the JIRA task id then or do we not need a unique
> > handle in this case at all?
> My original idea was to allow to get task from the TaskList by repositoryUrl and
> the key. I guess it would need to be done on case by case basis.

I guess I am misunderstanding this, but the task may not be in the task list at all?

We should consider making the following change in OpenRepositoryTaskJob to fix this the ambiguous task handle for JIRA:

Replace RepositoryTaskHandleUtil.getHandle(repository.getUrl(), taskId)
by RepositoryTaskHandleUtil.getHandle(repository.getUrl(), downloadedTaskData.getId()) .

Comment 35 Mik Kersten 2007-02-14 12:35:48 EST

I just removed almost all of the remaining references to RepositoryTaskHandleUtil that aren't in tests or in the few places that still rely on handle creation (e.g. internals refactoring handles, opening corresponding task when no task is present in the Task List).

Steffen: since it is getting too late for big changes for 2.0M1, can you make this work with either your escaping approach or the suggestion in comment#34?  Regarding the escaping, note that we are currently unable to change the stored handle format, since that would break old task contexts, history, etc.

Comment 36 Steffen Pingel 2007-02-14 12:41:39 EST

The escaping doesn't break anything because task ids in existing handles do not contain dashes. I can submit a patch later today.

Comment 37 Mik Kersten 2007-02-14 12:49:00 EST

Yup, if the escaping just does the "-" for IDs that works.  I assume that you'll put this escaping into RepositoryTaskHandleUtil.getHandle() ?

Comment 38 Eugene Kuleshov 2007-02-14 12:50:09 EST

This does not make sense. We do know that it is not a handle already. Why do we need to escape anything then?

Comment 39 Mik Kersten 2007-02-14 14:20:54 EST

Because we still have code that relies on parsing for the last occurrence of '-'.  So for 2.0M1 I can only think of two options:
1) Escape occurrences of '-' in the taskId.  However, not that JIRA should *not* be using the key as the taskId, because that will cause issues to lose identity when they move between components.  The actual ID shoudl be used, for the same reason that JIRA uses it internally--it uniquely identifies the issue no matter what its attributes are.
2) Instead of parsing for the last occurrence of '-' in RepositoryTaskHandleUtil, it may be possible to first find the repository URL, then use the rest of the string (less '-') as the handle.

For now I think the best thing to do is just fix this instance of the problem by Steffen doing what he proposed in comment#34, no?

Comment 40 Eugene Kuleshov 2007-02-14 14:38:03 EST

(In reply to comment #39)
> Because we still have code that relies on parsing for the last occurrence of
> '-'.  So for 2.0M1 I can only think of two options:
> 1) Escape occurrences of '-' in the taskId.  However, not that JIRA should
> *not* be using the key as the taskId, because that will cause issues to lose
> identity when they move between components.  The actual ID shoudl be used, for
> the same reason that JIRA uses it internally--it uniquely identifies the issue
> no matter what its attributes are.

I agree. But the point is that there should be alternative API for searching task in the task list without using or constructing handle. You even added such API, but it is internally constructing handle, which is why we have this problem.

Maybe we should instead search task in the task list by its web URL, that can be constructed by any connector from the repository URL and task key/id. I think it will be sufficient for the Open Corresponding Task use case.

> 2) Instead of parsing for the last occurrence of '-' in
> RepositoryTaskHandleUtil, it may be possible to first find the repository URL,
> then use the rest of the string (less '-') as the handle.
> 
> For now I think the best thing to do is just fix this instance of the problem
> by Steffen doing what he proposed in comment#34, no?

It won't help. You are trying to fix the erorr but not the cause. If you encode handle, TaskList won't find task for it and we do know that it won't find it.

Comment 41 Mik Kersten 2007-02-14 15:00:33 EST

Steffen's suggestion in comment#34 should work I think, it is to:
> We should consider making the following change in OpenRepositoryTaskJob to fix this the ambiguous task handle for JIRA:
> Replace RepositoryTaskHandleUtil.getHandle(repository.getUrl(), taskId) by RepositoryTaskHandleUtil.getHandle(repository.getUrl(), downloadedTaskData.getId()) .

Steffen: unless you have additional suggestions, I think that we should avoid encoding the handle.

Eugene: I agree that we want the alternate searching mechanism.  But I need to know if it's worth not adding other 2.0M1 improvements to get it.  If the above works, is the lack of search/lookup blocking anythign else?

Comment 42 Steffen Pingel 2007-02-14 15:17:30 EST

 (In reply to comment #41)
> Steffen: unless you have additional suggestions, I think that we should avoid
> encoding the handle.

Fine with me if we will support task ids with dashes in future version and opening of JIRA tasks is fixed somehow.

Comment 43 Eugene Kuleshov 2007-02-14 15:49:53 EST

(In reply to comment #41)
> Eugene: I agree that we want the alternate searching mechanism.  But I need to
> know if it's worth not adding other 2.0M1 improvements to get it.  If the above
> works, is the lack of search/lookup blocking anythign else?

Sorry, I am not sure what "above" you are referring to. Apparently new TaskList.getTask(respositoryUrl, taskId) method is not working and we have conflict with taskId vs. taskKey for JIRA. So, it seems like it will be better to replace new TaskList.getTask() method with something like TaskList.getTaskByWebUrl(taskUrl).

Comment 44 Mik Kersten 2007-02-14 21:07:27 EST

I can add TaskList.getTaskByWebUrl(taskUrl) tomorrow.  It will do a search for now.  Will that meet this use case?

By "above" I was referring to the first paragraph of comment#41.

The JIRA Connector should never use the key for the ID.  That's not happening anywhere, is it?

Comment 45 Steffen Pingel 2007-02-14 21:22:15 EST

 (In reply to comment #44)
> The JIRA Connector should never use the key for the ID.  That's not happening
> anywhere, is it?

If a key is entered into the open repository dialog the key would end up in the handle (right now it won't because it doesn't allow dashes). JiraTaskDataHandler.getJiraIssue() can actually keys as well as ids to download task data.

Comment 46 Eugene Kuleshov 2007-02-14 21:37:42 EST

(In reply to comment #44)
> I can add TaskList.getTaskByWebUrl(taskUrl) tomorrow.  It will do a search for
> now.  Will that meet this use case?

Not sure what search.

> By "above" I was referring to the first paragraph of comment#41.

I don't think it will help because we may have key in our hands, but no task data, so there will be no id to use.

> The JIRA Connector should never use the key for the ID.  That's not happening
> anywhere, is it?

That is exactly what is happening in Open Remote Task dialog and in Open Corresponding Task action. I guess for those actions, the only option is to make those things to use taskWebUrl for searching trough TaskList.

Comment 47 Steffen Pingel 2007-02-14 22:01:14 EST

I think we need to differentiate the different possible cases here:

Current implementation:

1) task is in task list: look up by handle
 TasksUiUtil.openRepositoryTask() -> TaskList.getTask()
 
 - Fails for JIRA keys since handle is composed of id

2) task is not in task list
 TasksUiUtil.openRepositoryTask() -> AbstractRepositoryConnecotr.openRepositoryTask()
 -> OpenRepositoryTaskJob.run()
 
 2a) task data available
 Open editor

 - Throws exception while constructing handle but would work for JIRA since task can be retrieved by key as well as id.
 
 2b) task data not available
 Open url: AbstractRepositoryConnecotr.getTaskWebUrl()

 - Works for keys but not for ids?

Comment 48 Eugene Kuleshov 2007-02-14 22:23:39 EST

Steffen, we probably should step to the starting point for those. At least for "Open Repository Task" and "Open Correspond Task" we do know repository url and key (not id). Both of those actions need to check if task is in the task list.

Comment 49 Steffen Pingel 2007-02-15 14:27:20 EST

> Steffen, we probably should step to the starting point for those. At least for
> "Open Repository Task" and "Open Correspond Task" we do know repository url and
> key (not id). Both of those actions need to check if task is in the task list.

Don't we need some type of mapping from keys to ids in AbstractConnector then?

Comment 50 Mik Kersten 2007-02-15 14:37:06 EST

We could definitely add that at some point, but currently keys are JIRA-specific, so it's not API yet.

Steffen: the policy you describe is in TasksUiUtil.openRepositoryTask(String repositoryUrl, String taskId, String fullUrl).  Can you just use that?  It will do the lookup based on URL if the key is not found, so I think it would make JiraHyperLink work?  But for both this and OpenRepositoryTask.openRemoteTask(..) you will need a mechanism for getting either the ID or the URL for a JIRA task given the key.  OpenRepositoryTask might need to be extended to use a connector so that it can work in a JIRA specific way.

Do you want to give this a try?  If you put that mechanism in place I could also help out.

Comment 51 Eugene Kuleshov 2007-02-15 14:46:54 EST

(In reply to comment #49)
> Don't we need some type of mapping from keys to ids in AbstractConnector then?

Not really. The use case is to be able to find existing Task by repository id and key (or by webTaskUrl, which is derived from those two an can be created by any connector). Because TaskList has list of tasks, all we need is to search trough those tasks. So, I think that searching by webTaskUrl will be less error prone and won't require TaskList to know about connectors.

Comment 52 Eugene Kuleshov 2007-02-15 14:52:55 EST

(In reply to comment #50)
> We could definitely add that at some point, but currently keys are
> JIRA-specific, so it's not API yet.

The API is AbstractRepositoryConnector.getTaskWebUrl(..) for JIRA it assumes key and not id. The reason is that this key is the identity that user see (for bugzilla that identity is the id).

> Steffen: the policy you describe is in TasksUiUtil.openRepositoryTask(String
> repositoryUrl, String taskId, String fullUrl).  Can you just use that?  It will
> do the lookup based on URL if the key is not found, so I think it would make
> JiraHyperLink work?  

It don't really need to know anything about key and can just use full task use wich will be the same as returned by AbstractRepositoryConnector.getTaskWebUrl()

> But for both this and
> OpenRepositoryTask.openRemoteTask(..) you will need a mechanism for getting
> either the ID or the URL for a JIRA task given the key.  OpenRepositoryTask
> might need to be extended to use a connector so that it can work in a JIRA
> specific way.

Once again, we can just use task web url, which can be constructed from repository url and id/key.

> Do you want to give this a try?  If you put that mechanism in place I could
> also help out.

So, the only missing piece is to search TaskList by webTaskUrl.

Comment 53 Mik Kersten 2007-02-15 15:16:49 EST

 (In reply to comment #52)
> So, the only missing piece is to search TaskList by webTaskUrl.

Done: TaskList.getRepositoryTask(String taskUrl).  Currently does a search, but we can consider maintaining a map if this ends up being used frequently.  I've updated TasksUiUtil.openRepositoryTask(String repositoryUrl, String taskId, String fullUrl) to use this method.  Let me know if you need anything else.

Comment 54 Mik Kersten 2007-02-16 13:32:42 EST

What's left on this?  I just checked and Open Corresponding Task is working from the synchronize view.  The thing that's failing with the usual exception, e.g. "invalid handle for task, can not contain: -, was: CAL-88" is hyperlinking in the JIRA Task Editor.  It would be really nice to have that working for 2.0M1.  We should be freezing early aft, but a localized change like this can come anytime before 4pm PST or so.

Eugene, Steffen, is this a starightforward fix for either of you guys?  Anything else broken, e.g. Open Corresponding Task from a comment?

Comment 55 Eugene Kuleshov 2007-02-16 14:31:51 EST

Created attachment 59188 [details]
Patch for TasksUiUtil

This patch is needed to make JIRA hyperlink detector work.

Comment 56 Eugene Kuleshov 2007-02-16 14:31:53 EST

Created attachment 59189 [details]
mylar/context/zip

Comment 57 Eugene Kuleshov 2007-02-16 14:42:38 EST

There is still an issue with OpenRepositoryTaskJob.run(), so Open Corresponding Task does not work in both Sync and History view

The following logic does not work for JIRA when job is invoked from History view:

				RepositoryTaskData downloadedTaskData = null;
				downloadedTaskData = offlineHandler.getTaskData(repository, taskId);
				if (downloadedTaskData != null) {
					TasksUiPlugin.getDefault().getTaskDataManager().push(RepositoryTaskHandleUtil.getHandle(repository.getUrl(), taskId), 
							downloadedTaskData);
				}
				openEditor(repository, RepositoryTaskHandleUtil.getHandle(repository.getUrl(), taskId), taskId, downloadedTaskData);

I think that downloadedTaskData should return handle id instead of calculating it using this bad bad bad method RepositoryTaskHandleUtil.getHandle().
Mik, this would be too big change for a patch. Can you please do that?

Comment 58 Mik Kersten 2007-02-16 15:38:30 EST

Patch didn't work because JiraHyperLink was not using the method you patched.  What I did for 2.0M1 is made JiraHyperLink.open() do the search and it works now, so it's great that we'll have your key-based hyperlinking out with 2.0M1.  Also, if you want to use it I added a TasksUiUtil.openRepositoryTask(String taskUrl) method that tries to open first with rich editor then with Browser.

			for (ITask task : TasksUiPlugin.getTaskListManager().getTaskList().getAllTasks()) {
				if (task instanceof JiraTask) {
					JiraTask jiraTask = (JiraTask)task;
					if (jiraTask.getKey() != null && jiraTask.getKey().equals(key)) {
						TasksUiUtil.refreshAndOpenTaskListElement(jiraTask);
					}
				}
			}
			
Rob: if there is time after 3.2 testing I'll get you to take a look at the problem when invoking from the History view.

Comment 59 Mik Kersten 2007-02-16 15:43:29 EST

Eugene: my bad, I didn't have your latest changes synched.  I'm not sure which implementation we should keep, so I'll just go with yours for now and override mine.

Comment 60 Eugene Kuleshov 2007-02-16 15:49:55 EST

(In reply to comment #58)
> Patch didn't work because JiraHyperLink was not using the method you patched. 

It would, if you'd updated from the CVS.

> What I did for 2.0M1 is made JiraHyperLink.open() do the search and it works
> now, so it's great that we'll have your key-based hyperlinking out with 2.0M1.

This bouncing back and forth is becoming really tiresome.

> Also, if you want to use it I added a TasksUiUtil.openRepositoryTask(String
> taskUrl) method that tries to open first with rich editor then with Browser.

Mik, you need to pass trough and really clean mess of TaskList.getTask*() and TasksUiUtil.open*() methods...

Comment 61 Mik Kersten 2007-02-16 15:59:59 EST

 (In reply to comment #60)
> Mik, you need to pass trough and really clean mess of TaskList.getTask*() and
> TasksUiUtil.open*() methods...

Agreed that the back-and-forth on this bug was annoying.

Yes, I added a note on that to that class earlier today.  On top of this we have scatterred logic throughout the clients of the TasksUiUtil methods.  Created bug 174509 for this.

You implementation was still failing for my tests, possibly because I have some old bogus JIRA tasks in my Task List without proper repositories mapped.  I temporarily put mine back because it is safer, and made a note that we should switch back in the code.

Comment 62 Eugene Kuleshov 2007-02-16 16:11:20 EST

(In reply to comment #61)
> You implementation was still failing for my tests, possibly because I have some
> old bogus JIRA tasks in my Task List without proper repositories mapped.  

AHA! Now you can try to delete them and maybe then you will realize what I have to deal with...

Comment 63 Eugene Kuleshov 2007-02-16 16:40:49 EST

 (In reply to comment #61)
> You implementation was still failing for my tests, possibly because I have some
> old bogus JIRA tasks in my Task List without proper repositories mapped.  I
> temporarily put mine back because it is safer, and made a note that we should
> switch back in the code.

Well, your implementation won't work either. You assuming that task for the found hyperlink should be in the task list. My implementation was capable of opening both existing and remote tasks, but may fail if there are some stale tasks (i.e. weblinked tasks or tasks from the web connector). Not sure how we can get around that, but we do know connector type and even repository for that tasks. So, maybe search should work only trough tasks from that repository, but that should be happening in the OpenRepositoryTaskJob and not in the JIRA hyperlink detector.

Comment 64 Steffen Pingel 2007-02-16 17:04:08 EST

> This bouncing back and forth is becoming really tiresome.

I agree and I am sorry it took me so long to understand the scope of the problem.

I have to admit though that I am not very happy with the solution we have now. The JIRA API is very unclear about when it expects a key and when an id. Furthermore we still have not solved the fundamental issues that are caused by the key/id ambiguity. Instead we have patched the symptoms by using the URL for lookup which happens to be constructed by key. 

The handle is constructed from a key provided by the user (it is even possible to create different handles for the same task using different casing) which is not good and still throws an exception. 

We won't get this fixed in 2.0M1 but I am going to take a shot at creating a patch this weekend that will use a mapping function as mentioned in comment 49. 

Comment 65 Mik Kersten 2007-02-16 17:11:39 EST

Steffen: I agree.  For now I think the mapping should be restricted to the JIRA Connector, since it is the only one I know of that has this duality but there could be others downstream so it will be a candidate for generalizing.  I actually think it is a good split that JIRA has beetween a robust ID and a human-readable/writable key.  Do you want to make a new bug for this?

Eugene: regarding comment#63: yes, good point, we have this "degraded functionality when task not in list limitation" pop up in a few other places too.  Created bug 174521.  

Comment 66 Eugene Kuleshov 2007-02-16 17:18:21 EST

(In reply to comment #65)
> Steffen: I agree.  For now I think the mapping should be restricted to the JIRA
> Connector...

I can't take this anymore.

Comment 67 Mik Kersten 2007-02-16 17:33:22 EST

 (In reply to comment #66)
> I can't take this anymore.

Clearly we have a misunderstanding, and if it's because of my rushing with this to get the other release bits done I apologize.  I can Skype with you for a bit so that we can sort out what I'm not understanding.

Comment 68 Eugene Kuleshov 2007-02-16 19:53:37 EST

(In reply to comment #64)
> We won't get this fixed in 2.0M1 but I am going to take a shot at creating a
> patch this weekend that will use a mapping function as mentioned in comment 49. 

Steffen, please don't do this! Each task does have web url, and each connector know how to construct that url from the user-readable key (or id). That url (and connector/repository type) is sufficient for retrieving task from the task list or even remote task. The problem we are suffering now is that common code is still constructing handles even for the cases when task instance is around and handle can be retrieved from the task.

PS: the weirdest part is that I've been waiting for about two months to see this fixed, but all it takes is to actually remove throwing this exception. So, even handle is invalid, it is usually used to get task from the task list, so there won't be task for such broken handle, but all common code fall into full task list scan after that anyways.

Comment 69 Steffen Pingel 2007-02-16 22:55:50 EST

 (In reply to comment #68)
> Each task does have web url, and each connector
> know how to construct that url from the user-readable key (or id). 

Basically, we are now using the URL as a unique identifier for tasks (IMHO we should get rid of handles then).

AbstractRepositoryConnector.getTaskWebUrl() and getTaskIdFromTaskUrl() are exactly the mapping functions is was talking about. These needs to be fixed for JIRA though. The former should work for handles as well as ids whereas the later should always return the id and never the key.

> PS: the weirdest part is that I've been waiting for about two months to see this
> fixed, but all it takes is to actually remove throwing this exception. So, even
> handle is invalid, it is usually used to get task from the task list, so there
> won't be task for such broken handle, but all common code fall into full task
> list scan after that anyways.

Allowing the construction of invalid handles does not seem like a good idea to me.

Comment 70 Mik Kersten 2007-02-16 23:06:25 EST

Let's discuss this during the meeting next Tuesday.  It's an important and tricky issue.  Just fyi, while connectors can dynamically create URLs to uniquely identify tasks, we can't use that format for persisting the identity (e.g. in contexts) because URL handles contain segments that are implementation details and can change (e.g. show_bug.cgi).

Comment 71 Eugene Kuleshov 2007-02-16 23:08:17 EST

(In reply to comment #69)
> Basically, we are now using the URL as a unique identifier for tasks (IMHO we
> should get rid of handles then).

It doesn't have to be unique.

> AbstractRepositoryConnector.getTaskWebUrl() and getTaskIdFromTaskUrl() are
> exactly the mapping functions is was talking about. These needs to be fixed for
> JIRA though. The former should work for handles as well as ids whereas the
> later should always return the id and never the key.

I am not quite sure about that. You will need server roundtrip or look trough TaskList to get id from JIRA task key. Since task data retrieveal will require key anyways, it seem not necessary.

> > PS: the weirdest part is that I've been waiting for about two months to see this
> > fixed, but all it takes is to actually remove throwing this exception. So, even
> > handle is invalid, it is usually used to get task from the task list, so there
> > won't be task for such broken handle, but all common code fall into full task
> > list scan after that anyways.
> 
> Allowing the construction of invalid handles does not seem like a good idea to
> me.

Well, I bet it would fix most of the current issues. I think it would be a better idea then wait another two months...

Comment 72 Steffen Pingel 2007-02-17 12:03:46 EST

> > AbstractRepositoryConnector.getTaskWebUrl() and getTaskIdFromTaskUrl() are
> > exactly the mapping functions is was talking about. These needs to be fixed
> for
> > JIRA though. The former should work for handles as well as ids whereas the
> > later should always return the id and never the key.
> I am not quite sure about that. You will need server roundtrip or look trough
> TaskList to get id from JIRA task key. Since task data retrieveal will require
> key anyways, it seem not necessary.

Good point. I was more arguing from an API point of view. I suggest that we make it clear by adding comments and renaming variables for JIRA when a key is expected (e.g. use these as variable names: keyOrId, key, id).

> > > PS: the weirdest part is that I've been waiting for about two months to see
> this
> > > fixed, but all it takes is to actually remove throwing this exception. So,
> even
> > > handle is invalid, it is usually used to get task from the task list, so
> there
> > > won't be task for such broken handle, but all common code fall into full
> task
> > > list scan after that anyways.
> >
> > Allowing the construction of invalid handles does not seem like a good idea to
> > me.
> Well, I bet it would fix most of the current issues. I think it would be a
> better idea then wait another two months...

I agree. I will attach a patch shortly that does not use the broken handle for the task editor input but constructs a "clean" handle.

Comment 73 Steffen Pingel 2007-02-17 12:26:04 EST

Created attachment 59228 [details]
Fixes opening of remote tasks for JIRA

This patch fixes the handle construction for remote tasks. It also escapes handles that contain dashes. Please note, that this does not mean that JIRA ever uses dashes to construct handles but other connectors might need to do that. For JIRA the task list lookup by handle will fail and it will fall back to downloading the task data.

The downside to handle escaping is that we may need to convert existing handles whenever deriving ids from handles is not required anymore and therefore the escaping can be dropped as well.

I will now shut up and stop adding to the confusion.

Comment 74 Eugene Kuleshov 2007-02-27 12:59:18 EST

Created attachment 59896 [details]
disable "corectness" in a favor to something that can be actually used

It seems like it is taking way to long to fix this issue. So, I am strongly suggesting to remove correctness check (as in the attached patch), so JIRA linking features will be actually usable. Mik, you can refactor it later to make it comletetly correct, but now we just have to make it work (at any price).

Also note, that commenting this makes open repository task and open correspong actions work fine for JIRA tasks. Though there is a weird bug, when opening corresponding JIRA task that is not in the Task List, task editor show internal JIRA id and not the key, which is completely confusing.

Comment 75 Mik Kersten 2007-02-27 16:00:04 EST

Most recent patch applied, next to most recent OpenRepositoryTaskJob part of patch applied.

Comment 76 Mik Kersten 2007-02-27 16:00:57 EST

Fyi, I did not have time to verify the behavior for JIRA.  Can do so on Friday.

Comment 77 Eugene Kuleshov 2007-02-27 16:06:47 EST

(In reply to comment #75)
> Most recent patch applied, next to most recent OpenRepositoryTaskJob part of
> patch applied.

Thanks Mik. Can you please put in a dev build with those changes and Rob's fix for JIRA detector for the text editors?

Comment 78 Steffen Pingel 2007-02-27 16:13:45 EST

I get an exception when opening a remote JIRA task. I can take a look later today.

org.eclipse.core.runtime.AssertionFailedException: assertion failed: 
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:109)
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:95)
	at org.eclipse.ui.part.MultiPageEditorPart.setActivePage(MultiPageEditorPart.java:690)
	at org.eclipse.ui.forms.editor.FormEditor.setActivePage(FormEditor.java:624)
	at org.eclipse.ui.part.MultiPageEditorPart.createPartControl(MultiPageEditorPart.java:282)
	[...]
	

Comment 79 Steffen Pingel 2007-02-28 01:13:09 EST

Added check to JiraTaskDataHandler if task was downloaded successfully and fixed the construction of handle that I missed in the original patch. I was able to open a remote JIRA task in an editor with these fixes.

I think the assertion is triggered when the web browser widget is disabled and no rich editor is available.

Comment 80 Steffen Pingel 2007-03-09 00:51:58 EST

*** Bug 174521 has been marked as a duplicate of this bug. ***

Comment 81 Mik Kersten 2007-04-09 23:17:26 EDT

As far as I can tell we're done here.  Remaining work related to this should go on bug 149624.