fix auth and move logic to router #1201

dpschen · 2021-12-15T19:08:36Z

dpschen commented

2021-12-15 19:08:36 +00:00

No description provided.

dpschen force-pushed feature/fix-auth from 414a5d6aa0 to c09cafbe6c

2021-12-15 19:12:15 +00:00

Compare

frederick commented

2021-12-15 19:15:36 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1201-featurefix-auth--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://1201-featurefix-auth--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.

konrad reviewed 2021-12-15 20:33:50 +00:00

src/components/home/contentAuth.vue

						
				@ -64,21 +64,25 @@ const route = useRoute()

				// hide menu on mobile

				watch(() => route.fullPath, () => window.innerWidth < 769 && store.commit(MENU_ACTIVE, false))

				// FIXME: this is really error prone

konrad commented

2021-12-15 20:32:04 +00:00

What makes it error prone?

konrad commented

2021-12-15 20:33:20 +00:00

Also why is this here and in the router definition file?

dpschen commented

2021-12-15 21:24:37 +00:00

It's not the same.

This seems to be a list of the routes where the currentList should be resetted.
It seems error prone because it is defined somewhere where you would not think of when creating a new view that does not have anything to do if the currentList.

Probably it would be better to reset the currentList always and then only set it again in the pages where there is one. What do you think?

Or have a route meta field where we define if the currentList is used. So my thought here is: instead of opting out the views should opt in that they want to show the currentList.

Then we do not need to reset it (I think) but can simply not show it.

It's not the same. This seems to be a list of the routes where the currentList should be resetted. It seems error prone because it is defined somewhere where you would not think of when creating a new view that does not have anything to do if the currentList. Probably it would be better to reset the currentList always and then only set it again in the pages where there is one. What do you think? Or have a route meta field where we define _if_ the currentList is used. So my thought here is: instead of opting out the views should opt in that they want to show the currentList. Then we do not need to reset it (I think) but can simply not show it.

dpschen commented

2021-12-15 21:27:02 +00:00

Regardless this pull request was intended as a quick fix. The above would be another issue.

👍 1

konrad marked this conversation as resolved

src/router/index.ts

						
				@ -0,0 +575,4 @@

					],

				})

				router.beforeEach((to) => {

konrad commented

2021-12-15 20:33:46 +00:00

Seems like this is much better!

dpschen commented

2021-12-15 21:28:22 +00:00

The stuff with router.resolve was just meant as an example for the error pages.
But when I read it I thought it was needed for them to work :D
Gladly the resolve function just looks up stuff and returns it (to nothing in this case) so it also didn't hurt :D

The stuff with router.resolve was just meant as an example for the error pages. But when I read it I thought it was needed for them to work :D Gladly the `resolve` function just looks up stuff and returns it (to nothing in this case) so it also didn't hurt :D

konrad marked this conversation as resolved

konrad commented

2021-12-15 20:34:10 +00:00

What's blocking this PR from being ready for merge? Looks fine to me.

👍 1

dpschen changed title from ~~WIP: fix auth and move logic to router~~ to fix auth and move logic to router

2021-12-15 21:28:51 +00:00

dpschen commented

2021-12-17 15:49:59 +00:00

Many tests fail :/

Seems to be a timing issue created by the additional time that is needed to check if a user is authorized to access each page :/

It's too many tests that fail right now. Any idea @konrad?

Many tests fail :/ Seems to be a timing issue created by the additional time that is needed to check if a user is authorized to access each page :/ It's too many tests that fail right now. Any idea @konrad?

konrad commented

2021-12-20 17:59:21 +00:00

It looks more like an issue with the notification library. All tests that fail assert that some global notification is shown after an action and that does not happen. I'm not sure why that would happen, the notification message seems to be correctly put together.

The notification does not get shown at all (and that makes the tests fail) but the rest seems to be working fine.

It looks more like an issue with the notification library. All tests that fail assert that some global notification is shown after an action and that does not happen. I'm not sure why that would happen, the notification message seems to be correctly put together. The notification does not get shown at all (and that makes the tests fail) but the rest seems to be working fine.

konrad reviewed 2021-12-21 07:38:26 +00:00

src/App.vue

						
				@ -7,4 +7,2 @@

							</template>

							<content-link-share v-else-if="authLinkShare"/>

							<content-no-auth v-else/>

							<notification/>