vikunja
/
vikunja
6
43
Fork 71
Code Issues 150 Pull Requests 14 Packages Releases 32 Activity

Trello Organization based Migration #2211

Merged
konrad merged 10 commits from Elscrux/vikunja:feature/trello-per-organization-migration into main 2024-04-09 10:55:09 +00:00
Conversation 7 Commits 10 Files Changed 2 +364 -215
Elscrux commented 2024-03-17 16:01:14 +00:00
Contributor
Copy Link

Migrate Trello organization after organization to limit total memory allocation.
Related discussion: https://community.vikunja.io/t/trello-import-issues/2110

Migrate Trello organization after organization to limit total memory allocation. Related discussion: https://community.vikunja.io/t/trello-import-issues/2110
Elscrux added 1 commit 2024-03-17 16:01:17 +00:00
konrad requested changes 2024-03-20 11:10:48 +00:00
Dismissed
konrad left a comment
Owner
Copy Link

Looks quite good already! Please also take a look at the failing CI and fix any errors it reported.

Looks quite good already! Please also take a look at the failing CI and fix any errors it reported.
pkg/modules/migration/trello/trello.go Outdated
@ -113,3 +109,1 @@
client := trello.NewClient(config.MigrationTrelloKey.GetString(), token)
client.Logger = log.GetLogger()
func getTrelloData(client *trello.Client) (trelloData []*trello.Board, err error) {
konrad commented 2024-03-20 11:04:22 +00:00
Owner
Copy Link

Can you rename that function? Since it now only gets the boards and not all data.

Can you rename that function? Since it now only gets the boards and not all data.
👍 1
konrad marked this conversation as resolved
pkg/modules/migration/trello/trello.go Outdated
@ -197,3 +211,3 @@
// Converts all previously obtained data from trello into the vikunja format.
// `trelloData` should contain all boards with their projects and cards respectively.
func convertTrelloDataToVikunja(trelloData []*trello.Board, token string) (fullVikunjaHierachie []*models.ProjectWithTasksAndBuckets, err error) {
func convertTrelloDataToVikunja(organizationName string, trelloData []*trello.Board, token string) (fullVikunjaHierachie []*models.ProjectWithTasksAndBuckets, err error) {
konrad commented 2024-03-20 11:10:10 +00:00
Owner
Copy Link

Is the token parameter still used?

Is the `token` parameter still used?
Elscrux commented 2024-03-24 23:37:00 +00:00
Author
Contributor
Copy Link

Yes, it was used here still.

buf, err := migration.DownloadFileWithHeaders(attachment.URL, map[string][]string{
  "Authorization": {`OAuth oauth_consumer_key="` + config.MigrationTrelloKey.GetString() + `", oauth_token="` + token + `"`},
})
Yes, it was used here still. ```go buf, err := migration.DownloadFileWithHeaders(attachment.URL, map[string][]string{ "Authorization": {`OAuth oauth_consumer_key="` + config.MigrationTrelloKey.GetString() + `", oauth_token="` + token + `"`}, }) ```
👍 1
konrad marked this conversation as resolved
pkg/modules/migration/trello/trello_test.go Outdated
@ -39,3 +39,3 @@
time1, err := time.Parse(time.RFC3339Nano, "2014-09-26T08:25:05Z")
require.NoError(t, err)
exampleFile, err := os.ReadFile(config.ServiceRootpath.GetString() + "/pkg/modules/migration/testimage.jpg")
exampleFile, err := os.ReadFile("E:\\dev\\vikunja\\pkg\\modules\\migration\\testimage.jpg")
konrad commented 2024-03-20 11:07:20 +00:00
Owner
Copy Link

You should set the VIKUNJA_SERVICE_ROOTPATH env variable, do not change this in the test.

You should set the `VIKUNJA_SERVICE_ROOTPATH` env variable, do not change this in the test.
Elscrux commented 2024-03-24 23:23:44 +00:00
Author
Contributor
Copy Link

Oops, sorry didn't mean to change this

Oops, sorry didn't mean to change this
konrad marked this conversation as resolved
pkg/modules/migration/trello/trello_test.go Outdated
@ -392,3 +415,1 @@
assert.NotNil(t, hierachie)
if diff, equal := messagediff.PrettyDiff(hierachie, expectedHierachie); !equal {
t.Errorf("converted trello data = %v, want %v, diff: %v", hierachie, expectedHierachie, diff)
organizationMap := createOrganizationMap(trelloData)
konrad commented 2024-03-20 11:09:40 +00:00
Owner
Copy Link

Can you add a test for the conversion happening in createOrganizationMap as well?

Can you add a test for the conversion happening in `createOrganizationMap` as well?
👍 1
konrad marked this conversation as resolved
Elscrux added 3 commits 2024-03-24 23:47:23 +00:00
continuous-integration/drone/pr Build is failing Details
5b502d3353 Add TestCreateOrganizationMap test
Elscrux force-pushed feature/trello-per-organization-migration from 5b502d3353 to 053f0a4c7a 2024-03-28 12:24:31 +00:00 Compare
Elscrux added 1 commit 2024-03-28 12:44:42 +00:00
continuous-integration/drone/pr Build is passing Details
91982d9d8a Fix lint error
frederick commented 2024-03-28 12:51:55 +00:00
Member
Copy Link

Hi Elscrux!

Thank you for creating a PR!

I've deployed the frontend changes of this PR on a preview environment under this URL: https://2211-feature-trello-per-organization---vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/.

This preview does not contain any changes made to the api, only the frontend.

Have a nice day!

Beep boop, I'm a bot.

Hi Elscrux! Thank you for creating a PR! I've deployed the frontend changes of this PR on a preview environment under this URL: https://2211-feature-trello-per-organization---vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/. This preview does not contain any changes made to the api, only the frontend. Have a nice day! > Beep boop, I'm a bot.
Elscrux commented 2024-03-28 12:53:20 +00:00
Author
Contributor
Copy Link

@konrad let me know if this works :)

I've got another PR to add non-upload attachments links to the description which I can start after this one is merged.

@konrad let me know if this works :) I've got another PR to add non-upload attachments links to the description which I can start after this one is merged.
konrad requested changes 2024-03-29 16:47:53 +00:00
Dismissed
konrad left a comment
Owner
Copy Link

I ran this locally and got the following error:

2024-03-29T17:45:23.250854365+01:00: DEBUG	▶ trello/Migrate 5d75 [Trello Migration] Getting organization with id  for user 1
2024-03-29T17:45:23.25090605+01:00: DEBUG	▶ trello/Get 5d76 [trello] GET organizations/?
2024-03-29T17:45:23.381500197+01:00: ERROR	▶ events/func1 5d7d Error while handling message 42968f36-ca7d-4ecf-8083-fe745869fb67, handler_poisoned=migration.requested.migration.listener, subscriber_poisoned=gochannel.GoChannel, reason_poisoned=HTTP request failure on https://api.trello.com/1/organizations/:
404: Cannot GET /1/organizations/?key=fooooooo&token=baaaaaar, topic_poisoned=migration.requested

It looks like the organization id is not populated properly?

I ran this locally and got the following error: ``` 2024-03-29T17:45:23.250854365+01:00: DEBUG ▶ trello/Migrate 5d75 [Trello Migration] Getting organization with id for user 1 2024-03-29T17:45:23.25090605+01:00: DEBUG ▶ trello/Get 5d76 [trello] GET organizations/? 2024-03-29T17:45:23.381500197+01:00: ERROR ▶ events/func1 5d7d Error while handling message 42968f36-ca7d-4ecf-8083-fe745869fb67, handler_poisoned=migration.requested.migration.listener, subscriber_poisoned=gochannel.GoChannel, reason_poisoned=HTTP request failure on https://api.trello.com/1/organizations/: 404: Cannot GET /1/organizations/?key=fooooooo&token=baaaaaar, topic_poisoned=migration.requested ``` It looks like the organization id is not populated properly?
pkg/modules/migration/trello/trello_test.go Outdated
@ -33,3 +33,3 @@
)
func TestConvertTrelloToVikunja(t *testing.T) {
func GetBoard(t *testing.T) ([]*trello.Board, time.Time) {
konrad commented 2024-03-29 16:33:53 +00:00
Owner
Copy Link

Please make this private so that it doesn't get used accidentally. And rename it to something like getTestBoard to make it clear that this should only be used for the test.

Please make this private so that it doesn't get used accidentally. And rename it to something like `getTestBoard` to make it clear that this should only be used for the test.
konrad marked this conversation as resolved
Elscrux added 1 commit 2024-03-29 18:20:05 +00:00
continuous-integration/drone/pr Build is failing Details
15b1c3e097 Make getBoard private
Elscrux force-pushed feature/trello-per-organization-migration from 15b1c3e097 to de31ffa899 2024-03-29 18:20:56 +00:00 Compare
Elscrux commented 2024-03-29 18:29:21 +00:00
Author
Contributor
Copy Link

I ran this locally and got the following error:

2024-03-29T17:45:23.250854365+01:00: DEBUG	▶ trello/Migrate 5d75 [Trello Migration] Getting organization with id  for user 1
2024-03-29T17:45:23.25090605+01:00: DEBUG	▶ trello/Get 5d76 [trello] GET organizations/?
2024-03-29T17:45:23.381500197+01:00: ERROR	▶ events/func1 5d7d Error while handling message 42968f36-ca7d-4ecf-8083-fe745869fb67, handler_poisoned=migration.requested.migration.listener, subscriber_poisoned=gochannel.GoChannel, reason_poisoned=HTTP request failure on https://api.trello.com/1/organizations/:
404: Cannot GET /1/organizations/?key=fooooooo&token=baaaaaar, topic_poisoned=migration.requested

It looks like the organization id is not populated properly?

Not sure what happens here. If the organizationID isn't correct, wouldn't it already panic in createOrganizationMap when accessing board.IDOrganization? All Trello boards should have this id afaik. I ran multiple migrations using this code, so sadly I'm not sure how I could fix this.

> I ran this locally and got the following error: > > ``` > 2024-03-29T17:45:23.250854365+01:00: DEBUG ▶ trello/Migrate 5d75 [Trello Migration] Getting organization with id for user 1 > 2024-03-29T17:45:23.25090605+01:00: DEBUG ▶ trello/Get 5d76 [trello] GET organizations/? > 2024-03-29T17:45:23.381500197+01:00: ERROR ▶ events/func1 5d7d Error while handling message 42968f36-ca7d-4ecf-8083-fe745869fb67, handler_poisoned=migration.requested.migration.listener, subscriber_poisoned=gochannel.GoChannel, reason_poisoned=HTTP request failure on https://api.trello.com/1/organizations/: > 404: Cannot GET /1/organizations/?key=fooooooo&token=baaaaaar, topic_poisoned=migration.requested > ``` > > It looks like the organization id is not populated properly? Not sure what happens here. If the organizationID isn't correct, wouldn't it already panic in `createOrganizationMap` when accessing `board.IDOrganization`? All Trello boards should have this id afaik. I ran multiple migrations using this code, so sadly I'm not sure how I could fix this.
Elscrux commented 2024-04-07 10:52:55 +00:00
Author
Contributor
Copy Link

@konrad Any ideas on this one? I'm not sure I can do anything to resolve this issue as I'm not experiencing it.

@konrad Any ideas on this one? I'm not sure I can do anything to resolve this issue as I'm not experiencing it.
konrad commented 2024-04-07 12:01:31 +00:00
Owner
Copy Link

I took a closer look and it seems like "Personal Boards" (whatever that is) do not belong to an organization and thus the IDOrganziation is an empty string. I would change the loop at pkg/modules/migration/trello/trello.go:423 so that it checks if the organization id is empty and in that case, puts it into a "Personal" pseudo-organization or something like that. Could also be done in createOrganizationMap.

I took a closer look and it seems like "Personal Boards" (whatever that is) do not belong to an organization and thus the `IDOrganziation` is an empty string. I would change the loop at pkg/modules/migration/trello/trello.go:423 so that it checks if the organization id is empty and in that case, puts it into a "Personal" pseudo-organization or something like that. Could also be done in `createOrganizationMap`.
konrad reviewed 2024-04-07 12:03:53 +00:00
pkg/modules/migration/trello/trello.go
@ -406,0 +446,4 @@
log.Debugf("[Trello Migration] Done migrating trello data for user %d for organization %s", u.ID, organizationID)
log.Debugf("[Trello Migration] Start inserting trello data for user %d for organization %s", u.ID, organizationID)
err = migration.InsertFromStructure(hierarchy, u)
konrad commented 2024-04-07 12:03:53 +00:00
Owner
Copy Link

Please use insertFromStructure from pkg/modules/migration/create_from_structure.go:48 (you need to make it public and change the name to something like InsertFromStructureWithSession and then create the session once before the loop and add the commit after the loop - rollback on any errors. You can wrap the whole loop in a new function which takes the session as an argument, to make it a bit less repetitive and having to do the rollback only once.

This has the advantage that you won't end up with a half-done migration when something fails, as in that case, it will be retried and then you might end up with the same projects multiple times.

Please use `insertFromStructure` from `pkg/modules/migration/create_from_structure.go:48` (you need to make it public and change the name to something like `InsertFromStructureWithSession` and then create the session once before the loop and add the commit after the loop - rollback on any errors. You can wrap the whole loop in a new function which takes the session as an argument, to make it a bit less repetitive and having to do the rollback only once. This has the advantage that you won't end up with a half-done migration when something fails, as in that case, it will be retried and then you might end up with the same projects multiple times.
konrad commented 2024-04-07 12:05:40 +00:00
Owner
Copy Link

On second thought, it could be nice to have partial migrations (some workspaces already migrated when only one is failing - as is happening currently in my case), but then we need to make sure the migration is not attempted again when it failed the first time. This would need to happen in the queue, might be more complicated.

On second thought, it could be nice to have partial migrations (some workspaces already migrated when only one is failing - as is happening currently in my case), but then we need to make sure the migration is not attempted again when it failed the first time. This would need to happen in the queue, might be more complicated.
Elscrux commented 2024-04-07 12:39:22 +00:00
Author
Contributor
Copy Link

Should it check what projects are already present in Vikunja somehow or is there some way to check what has been imported previously? With my current knowledge I have issues working on this as I don't understand all the concepts Vikunja has here. Can you give me some pointers or work on this yourself it that's easier?

Should it check what projects are already present in Vikunja somehow or is there some way to check what has been imported previously? With my current knowledge I have issues working on this as I don't understand all the concepts Vikunja has here. Can you give me some pointers or work on this yourself it that's easier?
konrad commented 2024-04-09 10:13:47 +00:00
Owner
Copy Link

TL;DR: should be all good now, with the implementation it currently has.

I've now changed how the retry-mechanism works and opted for a "fail early" model with notifications for the user if the migration fails. That should speed up the process when the migration fails, as it won't be retried as much.

In the specific case of this migration, this might mean partial migrations can happen, but I think it's an acceptable trade-off to have users manually deleting those. My original comment here led me to rethink this.

TL;DR: should be all good now, with the implementation it currently has. I've now [changed how the retry-mechanism works](https://kolaente.dev/vikunja/vikunja/commit/e10cd368bf5a167d5eb01edec854c901fbb1b806) and opted for a "fail early" model with notifications for the user if the migration fails. That should speed up the process when the migration fails, as it won't be retried as much. In the specific case of this migration, this might mean partial migrations can happen, but I think it's an acceptable trade-off to have users manually deleting those. My original comment here led me to rethink this.
konrad commented 2024-04-07 12:06:29 +00:00
Owner
Copy Link

Please take another look at the failing CI.

Please take another look at the failing CI.
Elscrux added 1 commit 2024-04-07 12:32:49 +00:00
Elscrux added 1 commit 2024-04-07 13:09:44 +00:00
continuous-integration/drone/pr Build is failing Details
673809d1ca Fix TestConvertTrelloToVikunja test
konrad added 1 commit 2024-04-09 10:04:26 +00:00
konrad added 1 commit 2024-04-09 10:26:47 +00:00
continuous-integration/drone/pr Build is passing Details
d686d417ae
chore: refactor names and variables
konrad added 1 commit 2024-04-09 10:40:25 +00:00
konrad approved these changes 2024-04-09 10:41:57 +00:00
konrad left a comment
Owner
Copy Link

I've fixed the last part where the migration would try to request an organization with id Personal - that won't work. Instead, it now uses the string as top level project name.

Should be good to merge now. Thanks a lot for this!

I've fixed the last part where the migration would try to request an organization with id `Personal` - that won't work. Instead, it now uses the string as top level project name. Should be good to merge now. Thanks a lot for this!
konrad scheduled this pull request to auto merge when all checks succeed 2024-04-09 10:42:42 +00:00
konrad merged commit 8458e77341 into main 2024-04-09 10:55:09 +00:00
Elscrux commented 2024-04-09 12:57:37 +00:00
Author
Contributor
Copy Link

Great, thanks for the final polish!

Great, thanks for the final polish!
Sign in to join this conversation.
Reviewers
No reviewers
konrad
Labels
Clear labels   
dependencies

   
duplicate

This issue or pull request already exists

  
help wanted

Need some help

  
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
needs reproduction

   
question

More information is needed

  
security

   
wontfix

This won't be fixed

No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#2211
No description provided.
Delete Branch "Elscrux/vikunja:feature/trello-per-organization-migration"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?