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

feat(webdav): initial implementation for a WebDAV simple auth provider #5528

Closed
wants to merge 8 commits into from

Conversation

dschmidt
Copy link
Contributor

As #4621 got stale and @Murderlon suggested to implement only the SimpleAuth part of it... here we go.

I intentionally left out any visual configuration for now, I would like to get the actual functionality merged asap and deal with everything else in a followup.

I couldn't make the translations work (at least not the ones in the AuthForm) in the dev server, any hints for that?
Otherwise it works for me (tm)

@dschmidt
Copy link
Contributor Author

Seen the type errors ... will fix tomorrow

@dschmidt dschmidt force-pushed the webdav-public-link-import branch from 52e2882 to ef88405 Compare November 29, 2024 09:55
@dschmidt
Copy link
Contributor Author

@Murderlon fixed the type issues, but I'm clueless about the compare_diff test... it looks like new dependencies should be installed by

corepack yarn workspaces focus $(\
corepack yarn workspaces list --json | \
jq -r .name | \
awk '/^@uppy-example/{ next } { if ($0!="uppy.io") print $0 }'\
)

but it doesn't seem to work?

@Murderlon Murderlon requested a review from mifi December 2, 2024 09:03
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Looks good overall! Thanks a lot for picking this up 👌

packages/@uppy/webdav/package.json Outdated Show resolved Hide resolved
packages/@uppy/webdav/package.json Outdated Show resolved Hide resolved
packages/@uppy/webdav/src/Webdav.tsx Show resolved Hide resolved
packages/@uppy/webdav/src/Webdav.tsx Outdated Show resolved Hide resolved
@Murderlon
Copy link
Member

I'm clueless about the compare_diff test

Perhaps reverting your changes for the action and resolving #5528 (comment) will help

@dschmidt dschmidt force-pushed the webdav-public-link-import branch from 0eea4ee to 1bbe4f8 Compare December 2, 2024 11:15
@dschmidt
Copy link
Contributor Author

dschmidt commented Dec 2, 2024

I'm clueless about the compare_diff test

Perhaps reverting your changes for the action and resolving #5528 (comment) will help

I'm afraid I can't easily change that (as commented above) - do you have any other ideas?
Also I'm still struggling with the translations as written in the PR text - any ideas here?

Apart from that everything seems to work and I've addressed all your comments (I think), thanks for the feedback!

@Murderlon
Copy link
Member

I'm afraid I can't easily change that (as commented above) - do you have any other ideas?
Also I'm still struggling with the translations as written in the PR text - any ideas here

I can take a look later

@Murderlon
Copy link
Member

Murderlon commented Dec 2, 2024

I can't make changes because you don't allow edits from contributors.

Try to add this to .github/workflows/e2e.yml (line 127)

        run: |
          git checkout FETCH_HEAD -- packages
+     - run: corepack yarn install
      - run: corepack yarn build:js:typeless

@dschmidt
Copy link
Contributor Author

dschmidt commented Dec 2, 2024

I can't make changes because you don't allow edits from contributors.

Maybe that's because I've sent the PR from the owncloud organization... mh :-\

Try to add this to .github/workflows/e2e.yml (line 127)

        run: |
          git checkout FETCH_HEAD -- packages
+     - run: corepack yarn install
      - run: corepack yarn build:js:typeless

Still failing :-\

@Murderlon
Copy link
Member

Let's undo f657f79 and I'll merge it. I'm pretty sure it's just problematic for PRs introducing a new dependency, which I can sort out in main and doesn't have to block this PR.

Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

Nice stuff!

  1. Should we have an e2e test for this or would that be very hard to implement? maybe there's a simple mock webdav server implemented in node.js or we can apt install a webdav server and start it up in cypress?
  2. should we have docs at https://github.com/transloadit/uppy.io ?
  3. ui not pretty but I guess we can merge it as-is for now. at least it works

packages/@uppy/webdav/src/Webdav.tsx Outdated Show resolved Hide resolved
.github/workflows/e2e.yml Outdated Show resolved Hide resolved
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
@dschmidt
Copy link
Contributor Author

dschmidt commented Dec 6, 2024

I will address @mifi 's comments asap, not sure what to do with the useCallback ... and I still need help with the translations, any hints?

@Murderlon
Copy link
Member

I still need help with the translations, any hints?

I'm traveling and away from my laptop till 16 December. @mifi would be great if you can help get this over the finish line.

@mifi
Copy link
Contributor

mifi commented Dec 7, 2024

I don't think you need to translate anything. You've added them English keys, and AFAIK it will fallback to those.

@dschmidt
Copy link
Contributor Author

dschmidt commented Dec 7, 2024

But it doesnt work! The keys are not picked up is what I'm saying

@dschmidt
Copy link
Contributor Author

@mifi ping, any help with the translations?

@dschmidt
Copy link
Contributor Author

dschmidt commented Dec 13, 2024

Incorporated your feedback @mifi, now only the translations aren't working yet afaict.

It would be great to have tests but not being familiar with your setup that would be quite some effort again for me and delay all of this further :-(

I guess we also could and should have docs, but I would tackle that in a follow up after having added a few customization options.
For now I would love to get the feature in to avoid merge conflicts and iterate then

@mifi
Copy link
Contributor

mifi commented Dec 14, 2024

@mifi ping, any help with the translations?

similar issue maybe? #5542

@Murderlon
Copy link
Member

@mifi ping, any help with the translations?

Sorry for the long wait, I was hoping this would be resolved while I was gone but I'm back now and can take a look.

@Murderlon Murderlon mentioned this pull request Dec 16, 2024
@mifi
Copy link
Contributor

mifi commented Dec 16, 2024

Sorry i had to prioritize node-sdk v4 last week

@dschmidt dschmidt closed this Dec 16, 2024
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.

3 participants