vikunja
/
vikunja
6
43
Fork 72
Code Issues 150 Pull Requests 19 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
2 changed files with 364 additions and 215 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

195
pkg/modules/migration/trello/trello.go
View File

@ -107,80 +107,100 @@ func (m *Migration) AuthURL() string {
"&return_url=" + config.MigrationTrelloRedirectURL.GetString()
}
konrad marked this conversation as resolved Outdated
konrad commented 2024-03-20 11:04:22 +00:00
Outdated
Review
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
func getTrelloData(token string) (trelloData []*trello.Board, err error) {
allArg := trello.Arguments{"fields": "all"}
client := trello.NewClient(config.MigrationTrelloKey.GetString(), token)
client.Logger = log.GetLogger()
func getTrelloBoards(client *trello.Client) (trelloData []*trello.Board, err error) {
log.Debugf("[Trello Migration] Getting boards...")
trelloData, err = client.GetMyBoards(trello.Defaults())
if err != nil {
return
return nil, err
}
log.Debugf("[Trello Migration] Got %d trello boards", len(trelloData))
for _, board := range trelloData {
log.Debugf("[Trello Migration] Getting projects for board %s", board.ID)
return
}
board.Lists, err = board.GetLists(trello.Defaults())
func getTrelloOrganizationsWithBoards(boards []*trello.Board) (boardsByOrg map[string][]*trello.Board) {
boardsByOrg = make(map[string][]*trello.Board)
for _, board := range boards {
// Trello boards without an organization are considered personal boards
if board.IDOrganization == "" {
board.IDOrganization = "Personal"
}
_, has := boardsByOrg[board.IDOrganization]
if !has {
boardsByOrg[board.IDOrganization] = []*trello.Board{}
}
boardsByOrg[board.IDOrganization] = append(boardsByOrg[board.IDOrganization], board)
}
return
}
func fillCardData(client *trello.Client, board *trello.Board) (err error) {
allArg := trello.Arguments{"fields": "all"}
log.Debugf("[Trello Migration] Getting projects for board %s", board.ID)
board.Lists, err = board.GetLists(trello.Defaults())
if err != nil {
return err
}
log.Debugf("[Trello Migration] Got %d projects for board %s", len(board.Lists), board.ID)
listMap := make(map[string]*trello.List, len(board.Lists))
for _, list := range board.Lists {
listMap[list.ID] = list
}
log.Debugf("[Trello Migration] Getting cards for board %s", board.ID)
cards, err := board.GetCards(allArg)
if err != nil {
return
}
log.Debugf("[Trello Migration] Got %d cards for board %s", len(cards), board.ID)
for _, card := range cards {
list, exists := listMap[card.IDList]
if !exists {
continue
}
card.Attachments, err = card.GetAttachments(allArg)
if err != nil {
return
}
log.Debugf("[Trello Migration] Got %d projects for board %s", len(board.Lists), board.ID)
listMap := make(map[string]*trello.List, len(board.Lists))
for _, list := range board.Lists {
listMap[list.ID] = list
}
log.Debugf("[Trello Migration] Getting cards for board %s", board.ID)
cards, err := board.GetCards(allArg)
if err != nil {
return nil, err
}
log.Debugf("[Trello Migration] Got %d cards for board %s", len(cards), board.ID)
for _, card := range cards {
list, exists := listMap[card.IDList]
if !exists {
continue
}
card.Attachments, err = card.GetAttachments(allArg)
if err != nil {
return nil, err
}
if len(card.IDCheckLists) > 0 {
for _, checkListID := range card.IDCheckLists {
checklist, err := client.GetChecklist(checkListID, allArg)
if err != nil {
return nil, err
}
checklist.CheckItems = []trello.CheckItem{}
err = client.Get("checklists/"+checkListID+"/checkItems", allArg, &checklist.CheckItems)
if err != nil {
return nil, err
}
card.Checklists = append(card.Checklists, checklist)
log.Debugf("Retrieved checklist %s for card %s", checkListID, card.ID)
if len(card.IDCheckLists) > 0 {
for _, checkListID := range card.IDCheckLists {
checklist, err := client.GetChecklist(checkListID, allArg)
if err != nil {
return err
}
}
list.Cards = append(list.Cards, card)
checklist.CheckItems = []trello.CheckItem{}
err = client.Get("checklists/"+checkListID+"/checkItems", allArg, &checklist.CheckItems)
if err != nil {
return err
}
card.Checklists = append(card.Checklists, checklist)
log.Debugf("Retrieved checklist %s for card %s", checkListID, card.ID)
}
}
log.Debugf("[Trello Migration] Looked for attachements on all cards of board %s", board.ID)
list.Cards = append(list.Cards, card)
}
log.Debugf("[Trello Migration] Looked for attachements on all cards of board %s", board.ID)
return
}
@ -196,7 +216,7 @@ func convertMarkdownToHTML(input string) (output string, err error) {
// 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) {
log.Debugf("[Trello Migration] ")
@ -205,7 +225,7 @@ func convertTrelloDataToVikunja(trelloData []*trello.Board, token string) (fullV
{
Project: models.Project{
ID: pseudoParentID,
Title: "Imported from Trello",
Title: organizationName,
},
},
}
@ -392,29 +412,58 @@ func (m *Migration) Migrate(u *user.User) (err error) {
log.Debugf("[Trello Migration] Starting migration for user %d", u.ID)
log.Debugf("[Trello Migration] Getting all trello data for user %d", u.ID)
trelloData, err := getTrelloData(m.Token)
client := trello.NewClient(config.MigrationTrelloKey.GetString(), m.Token)
client.Logger = log.GetLogger()
boards, err := getTrelloBoards(client)
if err != nil {
return
}
log.Debugf("[Trello Migration] Got all trello data for user %d", u.ID)
log.Debugf("[Trello Migration] Start converting trello data for user %d", u.ID)
fullVikunjaHierachie, err := convertTrelloDataToVikunja(trelloData, m.Token)
if err != nil {
return
organizationMap := getTrelloOrganizationsWithBoards(boards)
for organizationID, boards := range organizationMap {
log.Debugf("[Trello Migration] Getting organization with id %s for user %d", organizationID, u.ID)
orgName := organizationID
if organizationID != "Personal" {
organization, err := client.GetOrganization(organizationID, trello.Defaults())
if err != nil {
return err
}
orgName = organization.DisplayName
}
for _, board := range boards {
log.Debugf("[Trello Migration] Getting card data for board %s for user %d for organization %s", board.ID, u.ID, organizationID)
err = fillCardData(client, board)
if err != nil {
return err
}
log.Debugf("[Trello Migration] Got card data for board %s for user %d for organization %s", board.ID, u.ID, organizationID)
}
log.Debugf("[Trello Migration] Start converting trello data for user %d for organization %s", u.ID, organizationID)
konrad commented 2024-04-07 12:03:53 +00:00
Review
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
Review
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
Review
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
Review
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.
hierarchy, err := convertTrelloDataToVikunja(orgName, boards, m.Token)
if err != nil {
return err
}
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)
if err != nil {
return err
}
log.Debugf("[Trello Migration] Done inserting trello data for user %d for organization %s", u.ID, organizationID)
}
log.Debugf("[Trello Migration] Done migrating trello data for user %d", u.ID)
log.Debugf("[Trello Migration] Start inserting trello data for user %d", u.ID)
log.Debugf("[Trello Migration] Done migrating all trello data for user %d", u.ID)
err = migration.InsertFromStructure(fullVikunjaHierachie, u)
if err != nil {
return
}
log.Debugf("[Trello Migration] Done inserting trello data for user %d", u.ID)
log.Debugf("[Trello Migration] Migration done for user %d", u.ID)
return nil
return
}

384
pkg/modules/migration/trello/trello_test.go
View File

@ -32,20 +32,23 @@ import (
"github.com/stretchr/testify/require"
)
func TestConvertTrelloToVikunja(t *testing.T) {
func getTestBoard(t *testing.T) ([]*trello.Board, time.Time) {
konrad marked this conversation as resolved Outdated
konrad commented 2024-03-29 16:33:53 +00:00
Outdated
Review
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.
config.InitConfig()
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")
require.NoError(t, err)
konrad marked this conversation as resolved Outdated
konrad commented 2024-03-20 11:07:20 +00:00
Outdated
Review
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
Outdated
Review
Copy Link

Oops, sorry didn't mean to change this

Oops, sorry didn't mean to change this
trelloData := []*trello.Board{
{
Name: "TestBoard",
Desc: "This is a description",
Closed: false,
Name: "TestBoard",
Organization: trello.Organization{
ID: "orgid",
DisplayName: "TestOrg",
},
IDOrganization: "orgid",
Desc: "This is a description",
Closed: false,
Lists: []*trello.List{
{
Name: "Test Project 1",
@ -168,8 +171,13 @@ func TestConvertTrelloToVikunja(t *testing.T) {
},
},
{
Name: "TestBoard 2",
Closed: false,
Organization: trello.Organization{
ID: "orgid2",
DisplayName: "TestOrg2",
},
IDOrganization: "orgid2",
Name: "TestBoard 2",
Closed: false,
Lists: []*trello.List{
{
Name: "Test Project 4",
@ -183,8 +191,13 @@ func TestConvertTrelloToVikunja(t *testing.T) {
},
},
{
Name: "TestBoard Archived",
Closed: true,
Organization: trello.Organization{
ID: "orgid",
DisplayName: "TestOrg",
},
IDOrganization: "orgid",
Name: "TestBoard Archived",
Closed: true,
Lists: []*trello.List{
{
Name: "Test Project 5",
@ -197,67 +210,91 @@ func TestConvertTrelloToVikunja(t *testing.T) {
},
},
},
{
Name: "Personal Board",
Lists: []*trello.List{
{
Name: "Test Project 6",
Cards: []*trello.Card{
{
Name: "Test Card 5659",
Pos: 123,
},
},
},
},
},
}
trelloData[0].Prefs.BackgroundImage = "https://vikunja.io/testimage.jpg" // Using an image which we are hosting, so it'll still be up
expectedHierachie := []*models.ProjectWithTasksAndBuckets{
{
Project: models.Project{
ID: 1,
Title: "Imported from Trello",
},
},
{
Project: models.Project{
ID: 2,
ParentProjectID: 1,
Title: "TestBoard",
Description: "This is a description",
BackgroundInformation: bytes.NewBuffer(exampleFile),
},
Buckets: []*models.Bucket{
{
return trelloData, time1
}
func TestConvertTrelloToVikunja(t *testing.T) {
trelloData, time1 := getTestBoard(t)
exampleFile, err := os.ReadFile(config.ServiceRootpath.GetString() + "/pkg/modules/migration/testimage.jpg")
require.NoError(t, err)
expectedHierarchyOrg := map[string][]*models.ProjectWithTasksAndBuckets{
"orgid": {
{
Project: models.Project{
ID: 1,
Title: "Test Project 1",
},
{
ID: 2,
Title: "Test Project 2",
Title: "orgid",
},
},
Tasks: []*models.TaskWithComments{
{
Task: models.Task{
Title: "Test Card 1",
Description: "<p>Card Description <strong>bold</strong></p>\n",
BucketID: 1,
DueDate: time1,
Labels: []*models.Label{
{
Title: "Label 1",
HexColor: trelloColorMap["green"],
{
Project: models.Project{
ID: 2,
ParentProjectID: 1,
Title: "TestBoard",
Description: "This is a description",
BackgroundInformation: bytes.NewBuffer(exampleFile),
},
Buckets: []*models.Bucket{
{
ID: 1,
Title: "Test Project 1",
},
{
ID: 2,
Title: "Test Project 2",
},
},
Tasks: []*models.TaskWithComments{
{
Task: models.Task{
Title: "Test Card 1",
Description: "<p>Card Description <strong>bold</strong></p>\n",
BucketID: 1,
DueDate: time1,
Labels: []*models.Label{
{
Title: "Label 1",
HexColor: trelloColorMap["green"],
},
{
Title: "Label 2",
HexColor: trelloColorMap["orange"],
},
},
{
Title: "Label 2",
HexColor: trelloColorMap["orange"],
},
},
Attachments: []*models.TaskAttachment{
{
File: &files.File{
Name: "Testimage.jpg",
Mime: "image/jpg",
Size: uint64(len(exampleFile)),
FileContent: exampleFile,
Attachments: []*models.TaskAttachment{
{
File: &files.File{
Name: "Testimage.jpg",
Mime: "image/jpg",
Size: uint64(len(exampleFile)),
FileContent: exampleFile,
},
},
},
},
},
},
{
Task: models.Task{
Title: "Test Card 2",
Description: `
{
Task: models.Task{
Title: "Test Card 2",
Description: `
<h2> Checkproject 1</h2>
@ -270,117 +307,180 @@ func TestConvertTrelloToVikunja(t *testing.T) {
<ul data-type="taskList">
<li data-checked="false" data-type="taskItem"><label><input type="checkbox"><span></span></label><div><p>Pending Task</p></div></li>
<li data-checked="false" data-type="taskItem"><label><input type="checkbox"><span></span></label><div><p>Another Pending Task</p></div></li></ul>`,
BucketID: 1,
BucketID: 1,
},
},
},
{
Task: models.Task{
Title: "Test Card 3",
BucketID: 1,
{
Task: models.Task{
Title: "Test Card 3",
BucketID: 1,
},
},
},
{
Task: models.Task{
Title: "Test Card 4",
BucketID: 1,
Labels: []*models.Label{
{
Title: "Label 2",
HexColor: trelloColorMap["orange"],
{
Task: models.Task{
Title: "Test Card 4",
BucketID: 1,
Labels: []*models.Label{
{
Title: "Label 2",
HexColor: trelloColorMap["orange"],
},
},
},
},
},
{
Task: models.Task{
Title: "Test Card 5",
BucketID: 2,
Labels: []*models.Label{
{
Title: "Label 3",
HexColor: trelloColorMap["blue"],
},
{
Title: "Label 4",
HexColor: trelloColorMap["green_dark"],
},
{
Title: "Label 5",
HexColor: trelloColorMap["transparent"],
{
Task: models.Task{
Title: "Test Card 5",
BucketID: 2,
Labels: []*models.Label{
{
Title: "Label 3",
HexColor: trelloColorMap["blue"],
},
{
Title: "Label 4",
HexColor: trelloColorMap["green_dark"],
},
{
Title: "Label 5",
HexColor: trelloColorMap["transparent"],
},
},
},
},
},
{
Task: models.Task{
Title: "Test Card 6",
BucketID: 2,
DueDate: time1,
{
Task: models.Task{
Title: "Test Card 6",
BucketID: 2,
DueDate: time1,
},
},
{
Task: models.Task{
Title: "Test Card 7",
BucketID: 2,
},
},
{
Task: models.Task{
Title: "Test Card 8",
BucketID: 2,
},
},
},
{
Task: models.Task{
Title: "Test Card 7",
BucketID: 2,
},
{
Project: models.Project{
ID: 3,
ParentProjectID: 1,
Title: "TestBoard Archived",
IsArchived: true,
},
Buckets: []*models.Bucket{
{
ID: 3,
Title: "Test Project 5",
},
},
{
Task: models.Task{
Title: "Test Card 8",
BucketID: 2,
Tasks: []*models.TaskWithComments{
{
Task: models.Task{
Title: "Test Card 63423",
BucketID: 3,
},
},
},
},
},
{
Project: models.Project{
ID: 3,
ParentProjectID: 1,
Title: "TestBoard 2",
},
Buckets: []*models.Bucket{
{
ID: 3,
Title: "Test Project 4",
"orgid2": {
{
Project: models.Project{
ID: 1,
Title: "orgid2",
},
},
Tasks: []*models.TaskWithComments{
{
Task: models.Task{
Title: "Test Card 634",
BucketID: 3,
{
Project: models.Project{
ID: 2,
ParentProjectID: 1,
Title: "TestBoard 2",
},
Buckets: []*models.Bucket{
{
ID: 1,
Title: "Test Project 4",
},
},
Tasks: []*models.TaskWithComments{
{
konrad marked this conversation as resolved Outdated
konrad commented 2024-03-20 11:09:40 +00:00
Outdated
Review
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
Task: models.Task{
Title: "Test Card 634",
BucketID: 1,
},
},
},
},
},
{
Project: models.Project{
ID: 4,
ParentProjectID: 1,
Title: "TestBoard Archived",
IsArchived: true,
},
Buckets: []*models.Bucket{
{
ID: 4,
Title: "Test Project 5",
"Personal": {
{
Project: models.Project{
ID: 1,
Title: "Personal",
},
},
Tasks: []*models.TaskWithComments{
{
Task: models.Task{
Title: "Test Card 63423",
BucketID: 4,
{
Project: models.Project{
ID: 2,
ParentProjectID: 1,
Title: "Personal Board",
},
Buckets: []*models.Bucket{
{
ID: 1,
Title: "Test Project 6",
},
},
Tasks: []*models.TaskWithComments{
{
Task: models.Task{
Title: "Test Card 5659",
BucketID: 1,
},
},
},
},
},
}
hierachie, err := convertTrelloDataToVikunja(trelloData, "")
require.NoError(t, err)
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 := getTrelloOrganizationsWithBoards(trelloData)
for organizationID, boards := range organizationMap {
hierarchy, err := convertTrelloDataToVikunja(organizationID, boards, "")
require.NoError(t, err)
assert.NotNil(t, hierarchy)
if diff, equal := messagediff.PrettyDiff(hierarchy, expectedHierarchyOrg[organizationID]); !equal {
t.Errorf("converted trello data = %v,\nwant %v,\ndiff: %v", hierarchy, expectedHierarchyOrg[organizationID], diff)
}
}
}
func TestCreateOrganizationMap(t *testing.T) {
trelloData, _ := getTestBoard(t)
organizationMap := getTrelloOrganizationsWithBoards(trelloData)
expectedMap := map[string][]*trello.Board{
"orgid": {
trelloData[0],
trelloData[2],
},
"orgid2": {
trelloData[1],
},
"Personal": {
trelloData[3],
},
}
if diff, equal := messagediff.PrettyDiff(organizationMap, expectedMap); !equal {
t.Errorf("converted trello data = %v,\nwant %v,\ndiff: %v", organizationMap, expectedMap, diff)
}
}