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

Token urls #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Token urls #47

wants to merge 1 commit into from

Conversation

zerolab
Copy link

@zerolab zerolab commented Oct 25, 2021

This PR replaces #38 and fixes linting/test issues

@zerolab
Copy link
Author

zerolab commented Oct 26, 2021

@willbarton this is rebased on main, linted and the GitHub Action has GITHUB_TOKEN (https://github.com/cfpb/wagtail-sharing/blob/main/.github/workflows/test.yml#L75) but it still seems to fail on coveralls.

Looking at the docs, it seems they now recommend using https://github.com/marketplace/actions/coveralls-github-action and I wonder if that is the issue

@tombola tombola mentioned this pull request Nov 22, 2021
6 tasks
@zerolab
Copy link
Author

zerolab commented Feb 23, 2022

@chosak anything else to do on this? Would love to see it in, if you'll have it

setup.py Outdated
@@ -4,6 +4,7 @@
install_requires = [
"wagtail>=2.15,<4.0",
"django>=3.2,<5.0",
"pyjwt>1.7,<2.5",
Copy link
Author

Choose a reason for hiding this comment

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

this could potentially go to extras_require

extras_require = {
    "tokens": ["pyjwt>1.7,<2.5"]
}

katdom13 added a commit to katdom13/wagtail-sharing that referenced this pull request Nov 4, 2022
@nickmoreton
Copy link
Contributor

nickmoreton commented Feb 13, 2023

@zerolab FYI we just did some work on #61 for Wagtail 4.2

The same coverage fails are present: https://github.com/nickmoreton/wagtail-sharing/actions/runs/4165089219

@zerolab
Copy link
Author

zerolab commented Feb 13, 2023

The main blocker to getting this in is the user experience. e.g. can we make the sharing site optional when using token urls? is the wagtail_urls step still required? how do we explain the difference to users.

We had a catch up with @chosak towards the end of last year. I've been thinking of it and I wonder if want to split it up, and keep things light and focussed

@chosak
Copy link
Member

chosak commented Nov 20, 2023

Coming back to this from @nickmoreton's reminder on #69 (comment):

As @zeolab mentioned above, I'm a little unsure whether token URLs make sense alongside the existing sharing site approach or if what you have implemented there is actually mostly independent of our main wagtail-sharing use case. Our use case is as documented, letting e.g. https://draft.example.com/something/ show the draft version of the page at https://www.example.com/something/. The token URL approach instead seems to support https://www.example.com/drafts/?path=jwt - so the URL doesn't have to have the right page path or even use a sharing site at all. In that case I'm guessing the main value this package provides to you is the custom route logic here that lets you get at unpublished pages? And maybe the banner that indicates that you're viewing a draft?

If that's the case, I wonder if you could look at or comment on the issue I opened at wagtail/wagtail#10193 that asks whether there's some kind of standard way this logic could be introduced into Wagtail proper. We could definitely merge this here and have wagtail-sharing work in two mostly-unrelated ways that happen to share a bit of common logic. But it'd be nicer not to have to do that if there were some standard hook in Wagtail for doing this kind of thing.

@zerolab
Copy link
Author

zerolab commented Nov 21, 2023

@chosak I've added wagtail/wagtail#10193 to the next core meeting agenda.
I think the pragmatic thing to do in the interim is to create a fork that deals exclusively with the token URLs, with your permission. This way we don't have this long running PR, and we have a clear separation of concerns until we move wagtail/wagtail#10193 forward

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