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

Update createapplication command #1132

Merged
merged 13 commits into from
Mar 28, 2022

Conversation

vector-kerr
Copy link
Contributor

Fixes #
N/A

Description of the Change

Greetings!

This pull request updates the createapplication management command.

  • The --algorighm parameter is added which allows specification of "", "RS256", or "HS256"
  • The --skip-authorization help text is fixed
    • It was previously incorrectly described as "The ID of the new application"
    • It is now described as "If set, completely bypass the authorization form, even on the first use of the application"

Notes:

  • Documentation not updated because:
    • the command does not appear to be included in documentation
    • the command is self-documented (arguments and help strings displayed on invocation)

I hope you find this change useful!
- Daniel.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #1132 (294531c) into master (a62195b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1132   +/-   ##
=======================================
  Coverage   96.86%   96.87%           
=======================================
  Files          31       31           
  Lines        1789     1790    +1     
=======================================
+ Hits         1733     1734    +1     
  Misses         56       56           
Impacted Files Coverage Δ
..._provider/management/commands/createapplication.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a62195b...294531c. Read the comment docs.

@n2ygk n2ygk added this to the 2.0.0 milestone Mar 26, 2022
Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this!

Would you consider adding something to the docs for this command? It's never a bad thing to improve the documentation.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@vector-kerr
Copy link
Contributor Author

@n2ygk Sure thing, I've had a go at it in commit de0db70.

I'm definitely not an expert on RST documentation (much more familiar and comfortable with markdown) but I gave it a shot.

Someone will probably want to read over it to make sure I haven't buggered it up. I checked it in VSCode with an RST plugin and it seems ok, but I can't be certain.

- Daniel

@n2ygk
Copy link
Member

n2ygk commented Mar 27, 2022

@n2ygk Sure thing, I've had a go at it in commit de0db70.

I'm definitely not an expert on RST documentation (much more familiar and comfortable with markdown) but I gave it a shot.

I think most of us prefer markdown.

Someone will probably want to read over it to make sure I haven't buggered it up. I checked it in VSCode with an RST plugin and it seems ok, but I can't be certain.

To see what your docs edit looks like, you can locally run tox -e docs and then open docs/_build/html/index.html to see how it looks. You can similarly click on the Details for this check: docs/readthedocs.org:django-oauth-toolkit — Read the Docs build succeeded!

@n2ygk
Copy link
Member

n2ygk commented Mar 27, 2022

Weirdly the docs build failed even though it works locally.... I'll have to dig a bit further on that.

@n2ygk
Copy link
Member

n2ygk commented Mar 27, 2022

Weirdly the docs build failed even though it works locally.... I'll have to dig a bit further on that.

This project's tox setup doesn't refresh the .tox cache properly. After rm -r .tox/docs I can reproduce the error.

@n2ygk
Copy link
Member

n2ygk commented Mar 27, 2022

Weirdly the docs build failed even though it works locally.... I'll have to dig a bit further on that.

This project's tox setup doesn't refresh the .tox cache properly. After rm -r .tox/docs I can reproduce the error.

Unrelated problem: sphinx-doc/sphinx#10291 fixed in #1134.

@vector-kerr
Copy link
Contributor Author

Hello again @n2ygk,

I've just merged the master branch into my topical which I presume adds the version pin for jinja2.
If there's anything else you need from my end please let me know. :)

- Daniel

@n2ygk
Copy link
Member

n2ygk commented Mar 27, 2022

Hello again @n2ygk,

I've just merged the master branch into my topical which I presume adds the version pin for jinja2. If there's anything else you need from my end please let me know. :)

  • Daniel

Yeah I'll make sure to resync it with master as there are a few PRs in flight right now. Please either accept or reject the documentation suggestion I made w.r.t table vs. output of manage.py createapplication --help.

Thanks!

@vector-kerr
Copy link
Contributor Author

Please either accept or reject the documentation suggestion I made w.r.t table vs. output of manage.py createapplication --help.

Hello again @n2ygk,

Sorry to be a pain, but I don't seem to be able to find the documentation suggestion you're referring to.

GitHub shows me a big red "1 change requested" alert in this conversation, but when I click through with the "View changes" button, it just takes me to the review page where there are no unresolved review requests (that I can see). I also cannot see any obvious difference to the management_commands.rst documentation file that you might have made in the diff.

I do use GitLab a lot more than GitHub so I may also just not be looking in the right place...

I'm happy to look over it, I just can't seem to get to it!

Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

Sorry. I keep doing this. I left my suggestion Pending.

docs/management_commands.rst Outdated Show resolved Hide resolved
@vector-kerr
Copy link
Contributor Author

Sorry. I keep doing this. I left my suggestion Pending.

Not a problem!
Just committed your suggestion, so I think we're good to go.
Can I please get you to run the workflows once more? (I can't do it myself as a first-time contributor.)
Thanks for your patience and help!

- Daniel

@n2ygk n2ygk merged commit c8eee2c into jazzband:master Mar 28, 2022
@vector-kerr vector-kerr deleted the update-create-application-command branch March 28, 2022 13:37
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.

2 participants