Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing password form on public drop page #8007

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

JammingBen
Copy link
Contributor

Description

We've fixed a bug where the password form on a public drop page would not show after setting a required password.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

@JammingBen JammingBen self-assigned this Nov 21, 2022
@JammingBen JammingBen changed the title Fix mssing password form on public drop page Fix missing password form on public drop page Nov 21, 2022
@JammingBen JammingBen force-pushed the fix-missing-public-password-form branch from 1d3f873 to fddde38 Compare November 21, 2022 15:45
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is okay, I'm just a bit confused, why the earlier version did not work ... :-\

@JammingBen
Copy link
Contributor Author

The change is okay, I'm just a bit confused, why the earlier version did not work ... :-\

It seems set(vue, '$authService', authService) is not enough, vue.prototype.$authService = authService is needed as well to work (https://github.com/owncloud/web/blob/stable-6.0/packages/web-runtime/src/container/bootstrap.ts#L325) 🤔 Do you have an idea why though?

@@ -151,7 +152,7 @@ export default defineComponent({
.catch((error) => {
// likely missing password, redirect to public link password prompt
if (error.statusCode === 401) {
return this.$authService.handleAuthError(this.$router.currentRoute)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the authService not available anymore as this.$authService? 🤔

Copy link
Contributor Author

@JammingBen JammingBen Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #8007 (comment). I'm down to undo the change and rather fix it in announceAuthService, but I'm not sure why it's not working...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working with prototype seems to be the right way though: https://v2.vuejs.org/v2/cookbook/adding-instance-properties.html. I'll add it.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@@ -322,6 +322,7 @@ export const announceAuthService = ({
router: VueRouter
}): void => {
authService.initialize(configurationManager, clientService, store, router)
vue.prototype.$authService = authService
set(vue, '$authService', authService)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you can remove this line then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought so too, but it gives me: Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'initializeContext') 🤔 It's the same for $permissionManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a mess. You're a hero!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urgh... awesome 🤗

@kulmann kulmann merged commit f9a727e into stable-6.0 Nov 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix-missing-public-password-form branch November 22, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants