Change item id from int to long #59

michaelmarziani · 2022-07-08T09:39:24Z

The item id for newly created items no longer fits within an int, thus throwing an exception when such items are parsed.

Ldatz

forgive me if I'm incorrect. I'm looking at this code for the first time. Should public async Task int CloneItem(long itemId, bool silent = false) be Task long?

michaelmarziani · 2022-07-11T11:12:46Z

forgive me if I'm incorrect. I'm looking at this code for the first time. Should public async Task int CloneItem(long itemId, bool silent = false) be Task long?

@Ldatz Thanks for the review and I believe you're correct. I'll update the PR with this fix.

JonTvermose · 2022-07-18T09:14:17Z

@michaelmarziani There are still a couple of places in ItemService.cs where the itemId is referred as an int

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 191 in d90a1c7

public async Task<Item> GetItem(int itemId, bool markedAsViewed = true)

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 228 in d90a1c7

public async Task<Item> GetItemByAppItemId(int appId, int appItemId)

And for these two I believe the int arrays should be changed to long arrays.

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 475 in d90a1c7

    
           public async Task<List<Item>> GetTopValuesByField(int fieldId, int limit = 13, int[] notItemIds = null)

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 726 in d90a1c7

    
           public async Task<List<Item>> FindReferenceableItems(int fieldId, int limit = 13, int[] notItemIds = null,

@JonTvermose

Pointed out by @JonTvermose

michaelmarziani · 2022-07-18T09:51:20Z

@JonTvermose Thank you for the review. I agree with all the places you pointed out except the 2nd. The app item id is different than the global item id.

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 228 in d90a1c7

public async Task<Item> GetItemByAppItemId(int appId, int appItemId)

I've updated the PR to reflect the 3 fixes you pointed out.

JonTvermose · 2022-07-18T11:05:30Z

@michaelmarziani

This method probably needs an overload to accept long arrays :) Otherwise I suspect the code wont compile.

podio-dotnet/Source/Podio .NET/Utils/Utilities.cs

Line 13 in 6187c77

internal static string ArrayToCSV(int[] array, string splitter = ",")

Edit:
I also see some issues in CommentService.cs where the id can also be an itemId if the type specified is "item"

podio-dotnet/Source/Podio .NET/Services/CommentService.cs

Line 49 in 6187c77

public async Task<List<Comment>> GetCommentsOnObject(string type, int id)

podio-dotnet/Source/Podio .NET/Services/CommentService.cs

Line 79 in 6187c77

    
           public async Task<int> AddCommentToObject(string type, int id, string text, string externalId = null,

podio-dotnet/Source/Podio .NET/Services/CommentService.cs

Line 111 in 6187c77

    
           public async Task<int> AddCommentToObject(string type, int id, CommentCreateUpdateRequest comment, bool alertInvite = false,

There are probably more places as well, but those are the ones I've encountered so far.
I have created a fork of this repository and published a nuget package (https://www.nuget.org/packages/Tvermose.Podio.Async) as our production system has been hit by this error.. Hope things get fixed in this repository eventually so I can revert back and delete the fork :-)

michaelmarziani · 2022-07-18T15:24:35Z

@JonTvermose Would you be willing to send a PR based on your code? If so I'll cancel mine, as it looks like you've already done this work.

GuidoKuth · 2022-07-19T13:15:24Z

@JonTvermose Thank you for the review. I agree with all the places you pointed out except the 2nd. The app item id is different than the global item id.

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 228 in d90a1c7

public async Task<Item> GetItemByAppItemId(int appId, int appItemId)

I've updated the PR to reflect the 3 fixes you pointed out.

There seems to be one more place that have to be modified. Even if I saw in the code that this is already done it is not in your nuget package.

podio-dotnet/Source/Podio .NET/Services/ItemService.cs

Line 213 in d90a1c7

public async Task<Item> GetItemBasic(long itemId, bool markedAsViewed = true)

ManuBermudez17 · 2022-07-21T14:48:19Z

Do you know when they will commit a fix to this issue on podio/podio-dotnet?
Will someone publish these updates as packages on nuget?

michaelmarziani · 2022-07-21T16:34:17Z

@ManuBermudez17 My sense of things is that this is a purely speculative PR, which may some day be merged and re-released, or not. The reason for this is that this library is no longer officially maintained, the repo has been archived, and it's been tagged with "No maintenance intended".

Also fixes build problem by adding ArrayToCSV overload for a long[].

michaelmarziani · 2022-07-21T17:45:19Z

@JonTvermose Would you be willing to send a PR based on your code? If so I'll cancel mine, as it looks like you've already done this work.

Jon et al,

I ended up integrating all the fixes from your repository. You can see the current diff between what you have and what I have:
https://github.com/JonTvermose/podio-dotnet/compare/master..ioSlate:podio-dotnet:master

I believe my code is ready for review now, having integrated all the feedback.

ManuBermudez17 · 2022-07-21T18:45:00Z

@michaelmarziani I am using the synchronous way, I have tried the asynchronous way and I would have to change several things in the code. Is there any way to get some updated PR of the synchronous part to insert in my project? Because the packages that I see in nuget updated, are asynchronous. And I have tried some asynchronous one and I get several errors regarding the code I have.

michaelmarziani · 2022-07-21T19:08:22Z

@ManuBermudez17 I wish I could answer that. I noticed myself that when I rebuilt the code, all my sync methods became async and I had to rework them to work with the async code.

What I noticed is that there are 2 directories under Source, one named Podio .NET, the other Podio.Async. All the code is located in the Podio .NET directory, and the code looks like it's built to be async. There must be something in one of the project or solution files (there are many) that tells the code to build without async, but I don't know what it is.

If you have an hints or could point me in the right direction, I'd be happy to try to build a sync version and try to publish it to nuget.

ManuBermudez17 · 2022-07-21T19:38:04Z

@michaelmarziani I am like you. I have tried to compile from the Podio .NET folder, but as you say, it is asynchronous and several methods that I use fail with errors.

For example:

var filteredItems = podio.ItemService.FilterItems(appId: functionsPodio.appClientesId, filters: filter);
var filteredItemsString =  filteredItems.Filtered.ToString();

In Filtered it tells me that there is no accessible method with that name.

I have in my code:

using PodioAPI;
using PodioAPI.Utils.ItemFields;

Just like looping through a foreach, I did it like this:

foreach (var item in filteredItems.Items)
{
  ...
}

But also in Items, it tells me that there is no accessible method with that name.

And I can't find any manual on how to use the asynchronous way correctly.

ManuBermudez17 · 2022-07-21T19:50:10Z

@michaelmarziani I have found this branch v1, which I see that the last commits are from 2018 and I do not see the asynchronous folder, which may be the synchronous version. I would have to test it to see if it is valid for the project and I would have to update with the changes proposed by you.

Branch: https://github.com/podio/podio-dotnet/tree/v1/Source

ManuBermudez17 · 2022-07-21T20:11:42Z

I have tried the branch that I have previously posted, and it works for me with all the code that I had developed synchronously.

I would only have to test the changes made in the files that are named here, to see if everything is correct.

GuidoKuth · 2022-07-22T08:08:32Z

@JonTvermose I changed the nuget package from podio.async to yours mentioned above but there are some changes missing. Would you be willing to update your nuget package to reflect all latest changes. I think this would help. Thank you for your work.

michaelmarziani · 2022-07-22T08:42:39Z

I have tried the branch that I have previously posted, and it works for me with all the code that I had developed synchronously.

I would only have to test the changes made in the files that are named here, to see if everything is correct.

@ManuBermudez17 I looked at the v1 branch as well and it looks like up until July 7, 2018 both master and v1 were getting the same updates, so I agree that you could apply the same changes in this PR and use the v1 branch.

About the async code, I didn't have to change much, in case you're thinking of adapting your code to async. I had to change the method signature to return a Task for any methods that called Podio, and then await the Podio method, for example, sync code:

public int FetchItemFromPodio(Podio podio, long itemId)
{
    // Fetch the item
    var item = podio.ItemService.GetItemBasic(itemId);

    // Process the item and return 1 or 0 for success or failure
    // ...
}

Async code:

public async Task<int> FetchItemFromPodio(Podio podio, long itemId)
{
    // Fetch the item
    var item = await podio.ItemService.GetItemBasic(itemId);

    // Process the item and return 1 or 0 for success or failure
    // ...
}

Task is from System.Threading.Tasks.

JonTvermose · 2022-07-22T08:51:53Z

@JonTvermose I changed the nuget package from podio.async to yours mentioned above but there are some changes missing. Would you be willing to update your nuget package to reflect all latest changes. I think this would help. Thank you for your work.

@GuidoKuth Yes, I have updated the code and created a new package just now.

ManuBermudez17 · 2022-07-22T09:16:16Z

@michaelmarziani I have created a branch on my github, with only the synchronous part and the changes made to make it work properly. I have tried it in my project and no error is thrown.

The only thing left would be to create the nuget package, but I've never created one.

Repository: https://github.com/ManuBermudez17/podio-dotnet-sync

JonTvermose · 2022-07-22T09:31:31Z

@michaelmarziani I have created a branch on my github, with only the synchronous part and the changes made to make it work properly. I have tried it in my project and no error is thrown.

The only thing left would be to create the nuget package, but I've never created one.

Repository: https://github.com/ManuBermudez17/podio-dotnet-sync

@ManuBermudez17 If you have visual studio, it's not that difficult to create a nuget package.

https://docs.microsoft.com/en-us/nuget/quickstart/create-and-publish-a-package-using-visual-studio?tabs=netcore-cli

michaelmarziani · 2022-07-22T09:57:20Z

@AjmalVh @maarand @lowderjh when you have a spare moment, might I ask for a code review and approval if all looks good?

@JonTvermose Thanks for your advice and help.

GuidoKuth · 2022-07-22T10:41:17Z

@JonTvermose I changed the nuget package from podio.async to yours mentioned above but there are some changes missing. Would you be willing to update your nuget package to reflect all latest changes. I think this would help. Thank you for your work.

@GuidoKuth Yes, I have updated the code and created a new package just now.

Thank you very much @JonTvermose but I think there is still something missing. I get en error in ItmeService.FilterItems cause the returned items still have an int type itemId.

In the meantime I digged a littel bit deeper and I think the Problem is in the ItemField class where the FieldId ist still an int.

Ldatz · 2022-07-22T11:37:31Z

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

ManuBermudez17 · 2022-07-22T13:40:49Z

@Ldatz as @michaelmarziani commented, this package has the message that it will not have any more maintenance by Citrix.

https://developers.podio.com/clients/dotnet

michaelmarziani · 2022-07-22T14:28:16Z

@ManuBermudez17 That's true this package is marked as no maintenance intended, but one of the maintainers, @PavloBasiuk and I exchanged a comment on stackoverflow where they indicated that perhaps if I submitted a PR that it might be merged, but no promises.

In any case, at the end of the day at least we'll all have a nuget package that works, whether this repo or one of our forks. I really appreciate all the people that have joined together to work on sorting this out.

GuidoKuth · 2022-07-22T15:22:56Z

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

michaelmarziani · 2022-07-22T20:32:37Z

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

@GuidoKuth Can you post a snippet of the code that generates this error?

Ldatz · 2022-07-23T15:14:27Z

Where are you getting the error: System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."? After installing the Malte.Podio.Async NuGet package, I then had to change every occurrence of a call with an int item id to a long variable in my application that uses Podio.Async library.

GuidoKuth · 2022-07-24T15:22:55Z

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

@GuidoKuth Can you post a snippet of the code that generates this error?

Unfortunately not cause the error is thrown outside of my code.

GuidoKuth · 2022-07-24T15:24:40Z

Where are you getting the error: System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."? After installing the Malte.Podio.Async NuGet package, I then had to change every occurrence of a call with an int item id to a long variable in my application that uses Podio.Async library.

Yes, I have done so. The error seems to be thrown in the library itself cause the error says in first line Thrown by external code.

GuidoKuth · 2022-07-25T13:57:54Z

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

@GuidoKuth Can you post a snippet of the code that generates this error?

Unfortunately not cause the error is thrown outside of my code.

I am very sorry for the confusion but as my project uses man packages and is deeply structured there was another Package Reference for Podio that caused the error. I changed the package reference to the Malte.Podio.Async nuget Package and everything works perfect. Again sorry for confusion and thanks to all for your help.

michaelmarziani · 2022-08-04T21:09:31Z

Greetings collaborators, @lowderjh thank you for your approval and I wanted to ask if one more reviewer could give a final review and approval here if all looks good.

JonTvermose · 2022-08-17T10:50:44Z

I have released a new version of my fork where the Comment Id has also been changed to a long.
https://www.nuget.org/packages/Tvermose.Podio.Async

Github repo: https://github.com/JonTvermose/podio-dotnet

I have no affiliation with Podio, the fork exists as my production code is dependent on this library.

GuidoKuth · 2022-10-11T07:28:23Z

I changed all model classes from int to long Holen Sie sich Outlook für Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Louis Datz ***@***.***> Sent: Saturday, July 23, 2022 5:14:37 PM To: podio/podio-dotnet ***@***.***> Cc: Guido Kuth ***@***.***>; Mention ***@***.***> Subject: Re: [podio/podio-dotnet] Change item id from int to long (PR #59) Where are you getting the error: System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."? After installing the Malte.Podio.Async NuGet package, I then had to change every occurrence of a call with an int item id to a long variable in my application that uses Podio.Async library. — Reply to this email directly, view it on GitHub<#59 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABRWDSPGEW5NWSAX6MIQCQ3VVQD53ANCNFSM53AMMG7Q>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Change item id from int to long

69d2c90

Ldatz reviewed Jul 8, 2022

View reviewed changes

AjmalVh previously approved these changes Jul 9, 2022

View reviewed changes

maarand previously approved these changes Jul 11, 2022

View reviewed changes

lowderjh previously approved these changes Jul 11, 2022

View reviewed changes

Return value should be a long as this is an item_id

d90a1c7

michaelmarziani dismissed stale reviews from lowderjh, maarand, and AjmalVh via d90a1c7 July 11, 2022 11:18

Additional places item Id needs to be changed from int to long

3ea74d2

Pointed out by @JonTvermose

michaelmarziani added 2 commits July 21, 2022 19:23

Additional int to long changes in Comment service

9612e24

Also fixes build problem by adding ArrayToCSV overload for a long[].

One more int to long in the Comment service

b061c5f

lowderjh previously approved these changes Jul 25, 2022

View reviewed changes

madslund previously approved these changes Aug 11, 2022

View reviewed changes

More comment id int -> long

3e41562

michaelmarziani dismissed stale reviews from madslund and lowderjh via 3e41562 March 7, 2023 16:45

Fix to comment.ref.id unable to parse as int

5fd6979

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change item id from int to long #59

Change item id from int to long #59

michaelmarziani commented Jul 8, 2022

Ldatz left a comment •

edited

Loading

michaelmarziani commented Jul 11, 2022

JonTvermose commented Jul 18, 2022

michaelmarziani commented Jul 18, 2022

JonTvermose commented Jul 18, 2022 •

edited

Loading

michaelmarziani commented Jul 18, 2022

GuidoKuth commented Jul 19, 2022

ManuBermudez17 commented Jul 21, 2022 •

edited

Loading

michaelmarziani commented Jul 21, 2022

michaelmarziani commented Jul 21, 2022

ManuBermudez17 commented Jul 21, 2022

michaelmarziani commented Jul 21, 2022

ManuBermudez17 commented Jul 21, 2022

ManuBermudez17 commented Jul 21, 2022 •

edited

Loading

ManuBermudez17 commented Jul 21, 2022

GuidoKuth commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

JonTvermose commented Jul 22, 2022

ManuBermudez17 commented Jul 22, 2022

JonTvermose commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

GuidoKuth commented Jul 22, 2022 •

edited

Loading

Ldatz commented Jul 22, 2022

ManuBermudez17 commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

GuidoKuth commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

Ldatz commented Jul 23, 2022

GuidoKuth commented Jul 24, 2022

GuidoKuth commented Jul 24, 2022

GuidoKuth commented Jul 25, 2022

michaelmarziani commented Aug 4, 2022

JonTvermose commented Aug 17, 2022

GuidoKuth commented Oct 11, 2022 via email

Change item id from int to long #59

Are you sure you want to change the base?

Change item id from int to long #59

Conversation

michaelmarziani commented Jul 8, 2022

Ldatz left a comment • edited Loading

Choose a reason for hiding this comment

michaelmarziani commented Jul 11, 2022

JonTvermose commented Jul 18, 2022

michaelmarziani commented Jul 18, 2022

JonTvermose commented Jul 18, 2022 • edited Loading

michaelmarziani commented Jul 18, 2022

GuidoKuth commented Jul 19, 2022

ManuBermudez17 commented Jul 21, 2022 • edited Loading

michaelmarziani commented Jul 21, 2022

michaelmarziani commented Jul 21, 2022

ManuBermudez17 commented Jul 21, 2022

michaelmarziani commented Jul 21, 2022

ManuBermudez17 commented Jul 21, 2022

ManuBermudez17 commented Jul 21, 2022 • edited Loading

ManuBermudez17 commented Jul 21, 2022

GuidoKuth commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

JonTvermose commented Jul 22, 2022

ManuBermudez17 commented Jul 22, 2022

JonTvermose commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

GuidoKuth commented Jul 22, 2022 • edited Loading

Ldatz commented Jul 22, 2022

ManuBermudez17 commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

GuidoKuth commented Jul 22, 2022

michaelmarziani commented Jul 22, 2022

Ldatz commented Jul 23, 2022

GuidoKuth commented Jul 24, 2022

GuidoKuth commented Jul 24, 2022

GuidoKuth commented Jul 25, 2022

michaelmarziani commented Aug 4, 2022

JonTvermose commented Aug 17, 2022

GuidoKuth commented Oct 11, 2022 via email

Ldatz left a comment •

edited

Loading

JonTvermose commented Jul 18, 2022 •

edited

Loading

ManuBermudez17 commented Jul 21, 2022 •

edited

Loading

ManuBermudez17 commented Jul 21, 2022 •

edited

Loading

GuidoKuth commented Jul 22, 2022 •

edited

Loading