vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat: MigrateService script setup #2432

Merged
konrad merged 1 commits from dpschen/frontend:feature/feat-MigrationService-script-setup into main 2022-11-03 14:19:44 +00:00
Conversation 10 Commits 1 Files Changed 12 +105 -111
dpschen commented 2022-09-27 20:30:40 +00:00
Member
Copy Link
No description provided.
frederick commented 2022-09-27 20:33:48 +00:00
Member
Copy Link

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2432-feature-feat-migrationservice-sc--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 somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2432-feature-feat-migrationservice-sc--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 somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen changed title from WIP: feat: MigrateService script setup to feat: MigrateService script setup 2022-09-28 08:15:13 +00:00
dpschen commented 2022-09-28 08:15:48 +00:00
Author
Member
Copy Link

Should we align the naming? Because we have MigrateService and MigrationService

Should we align the naming? Because we have `MigrateService` and `MigrationService`
konrad was assigned by dpschen 2022-09-28 08:16:32 +00:00
konrad commented 2022-09-28 13:33:55 +00:00
Owner
Copy Link

I'd like to keep a distiction between the component and the service (the thing doing the requests to actually migrate). Maybe we should rename the component so that it does not contain Service?

I'd like to keep a distiction between the component and the service (the thing doing the requests to actually migrate). Maybe we should rename the component so that it does not contain `Service`?
dpschen commented 2022-09-28 15:40:39 +00:00
Author
Member
Copy Link

Simplest pattern I can think of:

Migrate.vue => Migration.vue
MigrateService.vue => MigrationHandler.vue

Other names I came up with:

Migrate.vue:

  • MigrationStart
  • MigrationSelection
  • MigrationOverview
  • MigrationIndex

MigrateService.vue:

  • Migration.vue (if the other file is not already called that)
  • MigrationHandler.vue

I wanted to make clear that it is in the latter component where the actual migration happens, thus the renaming of the first component.

Simplest pattern I can think of: `Migrate.vue` => `Migration.vue` `MigrateService.vue` => `MigrationHandler.vue` ----- Other names I came up with: `Migrate.vue`: - `MigrationStart` - `MigrationSelection` - `MigrationOverview` - `MigrationIndex` `MigrateService.vue`: - `Migration.vue` (if the other file is not already called that) - `MigrationHandler.vue` I wanted to make clear that it is in the latter component where the actual migration happens, thus the renaming of the first component.
konrad commented 2022-09-28 15:50:04 +00:00
Owner
Copy Link

Simplest pattern I can think of:

Migrate.vue => Migration.vue
MigrateService.vue => MigrationHandler.vue

I like that.

> Simplest pattern I can think of: > > `Migrate.vue` => `Migration.vue` > `MigrateService.vue` => `MigrationHandler.vue` > I like that.
dpschen force-pushed feature/feat-MigrationService-script-setup from 4461ef2f78 to 743b46cf8a 2022-09-28 20:43:09 +00:00 Compare
dpschen requested review from konrad 2022-09-29 00:47:22 +00:00
konrad approved these changes 2022-09-29 10:39:08 +00:00
konrad requested changes 2022-09-29 10:43:04 +00:00
konrad left a comment
Owner

Looks like the "get started" button does not do anything anymore:

image

Looks like the "get started" button does not do anything anymore: ![image](/attachments/b48b2bca-af1d-47a7-81f7-a2d0b8fdf120)
image.png
26 KiB
konrad commented 2022-09-29 10:45:36 +00:00
Owner
Copy Link

Navigating back (with the browser's back button) breaks:

image

Navigating back (with the browser's back button) breaks: ![image](/attachments/46524e2d-1c40-4336-b59d-c2cfef411ba3)
image.png
24 KiB
konrad reviewed 2022-09-29 10:46:41 +00:00
src/views/migrate/MigrationHandler.vue Outdated
@ -0,0 +122,4 @@
return
}
authUrl.value = await migrationService.getAuthUrl().then(({url}) => url)
konrad commented 2022-09-29 10:46:41 +00:00
Owner

It looks like this call to the api happens but the authUrl stays empty.

It looks like this call to the api happens but the `authUrl` stays empty.
dpschen reviewed 2022-09-29 11:54:35 +00:00
src/views/migrate/MigrationHandler.vue Outdated
@ -0,0 +150,4 @@
lastMigrationDate.value = null
message.value = ''
let migrationConfig : MigrationConfig = {code: migratorAuthCode.value}
dpschen commented 2022-09-29 11:54:35 +00:00
Author
Member

@konrad:
This MigrationConfig here is wrong. Since the config is passed to the migrate method of the service which passes it to update, which wants a File (?) I'm not sure what type to define here. Any idea?

@konrad: This `MigrationConfig` here is wrong. Since the config is passed to the `migrate` method of the service which passes it to update, which wants a `File` (?) I'm not sure what type to define here. Any idea?
konrad commented 2022-09-29 11:56:55 +00:00
Owner

Maybe MigrationConfig | File? The File type is correct but only when the migrator requires a file to be uploaded. Otherwise it needs that code to make api calls to the third party api.

Maybe `MigrationConfig | File`? The `File` type is correct but only when the migrator requires a file to be uploaded. Otherwise it needs that code to make api calls to the third party api.
dpschen commented 2022-09-29 12:00:04 +00:00
Author
Member

The update function is defined in the AbstractService thus it's types as well.
Have to find out how we can pass types there so that it supports different types depending on the use-case. I think it doesn't make sense to support the MigrationConfig type there.

The update function is defined in the `AbstractService` thus it's types as well. Have to find out how we can pass types there so that it supports different types depending on the use-case. I think it doesn't make sense to support the MigrationConfig type there.
dpschen force-pushed feature/feat-MigrationService-script-setup from 743b46cf8a to 9734bf23d9 2022-09-29 11:56:36 +00:00 Compare
dpschen commented 2022-09-29 12:16:11 +00:00
Author
Member
Copy Link

Navigating back (with the browser's back button) breaks:

image

This is interesting. Seems like the beforeRouteEnter doesn't get triggered.
Can you explain your flow? From which page did you navigate back from?

> Navigating back (with the browser's back button) breaks: > > ![image](/attachments/46524e2d-1c40-4336-b59d-c2cfef411ba3) This is interesting. Seems like the `beforeRouteEnter` doesn't get triggered. Can you explain your flow? From which page did you navigate back from?
konrad commented 2022-09-29 12:29:23 +00:00
Owner
Copy Link

Can you explain your flow? From which page did you navigate back from?

From /migrate/todoist (the page with the button I mentioned earlier).

> Can you explain your flow? From which page did you navigate back from? From `/migrate/todoist` (the page with the button I mentioned earlier).
dpschen force-pushed feature/feat-MigrationService-script-setup from 9734bf23d9 to 9fbba878b5 2022-10-03 19:45:29 +00:00 Compare
dpschen force-pushed feature/feat-MigrationService-script-setup from 9fbba878b5 to 7ad3e1c158 2022-10-28 20:23:26 +00:00 Compare
dpschen force-pushed feature/feat-MigrationService-script-setup from 7ad3e1c158 to f0f00068c4 2022-10-28 20:53:20 +00:00 Compare
dpschen force-pushed feature/feat-MigrationService-script-setup from f0f00068c4 to 4564da7bfe 2022-10-31 18:27:39 +00:00 Compare
konrad requested changes 2022-11-02 16:17:52 +00:00
konrad left a comment
Owner

After migrating, this should show the message (not the json):

image

After migrating, this should show the message (not the json): ![image](/attachments/6d2f580d-b098-4be2-b0d1-d5994c7a8554)
image.png
6.4 KiB
image.png
6.4 KiB
image.png
dpschen force-pushed feature/feat-MigrationService-script-setup from 4564da7bfe to 102923295f 2022-11-02 17:06:03 +00:00 Compare
dpschen force-pushed feature/feat-MigrationService-script-setup from 102923295f to daba3cfe4f 2022-11-02 17:11:12 +00:00 Compare
dpschen requested review from konrad 2022-11-02 17:11:13 +00:00
dpschen commented 2022-11-02 17:11:22 +00:00
Author
Member
Copy Link

Should be fixed

Should be fixed
konrad requested changes 2022-11-03 11:29:23 +00:00
konrad left a comment
Owner

Clicking on either one of the new lists in the menu or the "refresh" button brings up this error:

image

Clicking on either one of the new lists in the menu or the "refresh" button brings up this error: ![image](/attachments/ab025d9a-34eb-4513-8876-0125ed5742d8)
image.png
25 KiB
dpschen force-pushed feature/feat-MigrationService-script-setup from daba3cfe4f to ea30bed822 2022-11-03 12:25:02 +00:00 Compare
dpschen commented 2022-11-03 12:26:16 +00:00
Author
Member
Copy Link

Interesting. Seems like when passing the route props via the view component as parameter it doesn't change when the route changes.

When watching the route directly it does so before the route changes. Because of the route based properties get retriggered:

image

image

Solution seems to be to always watch the route via a watched param from the component.

I always prefered this approach but it also has its downsides, since there is an initial parsing of the route in the route config which bloats the overall app bundle, since every new page potentially adds more config).

I fixed the component that way. We probably have similar issues in other parts of the app. Potentially everywhere where we use useRoute instead of passing some route stuff via the props to the component.

Edit:
To get to the error state shown in the screens I did the steps that you described.

Interesting. Seems like when passing the route props via the view component as parameter it doesn't change when the route changes. When watching the route directly it does so before the route changes. Because of the route based properties get retriggered: ![image](/attachments/16cd5c72-849f-4d5b-b419-52f079d341ff) ![image](/attachments/f45fa432-53da-4218-b34b-a5113902aa6e) Solution seems to be to always watch the route via a watched param from the component. I always prefered this approach but it also has its downsides, since there is an initial parsing of the route in the route config which bloats the overall app bundle, since every new page potentially adds more config). I fixed the component that way. We probably have similar issues in other parts of the app. Potentially everywhere where we use `useRoute` instead of passing some route stuff via the props to the component. **Edit:** To get to the error state shown in the screens I did the steps that you described.
image.png
184 KiB
image.png
134 KiB
dpschen requested review from konrad 2022-11-03 12:26:23 +00:00
konrad approved these changes 2022-11-03 14:19:21 +00:00
konrad merged commit 8b7b4d61a3 into main 2022-11-03 14:19:44 +00:00
konrad deleted branch feature/feat-MigrationService-script-setup 2022-11-03 14:19:45 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
3 Participants
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/frontend#2432
No description provided.
Delete Branch "dpschen/frontend:feature/feat-MigrationService-script-setup"

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?