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.

Sign up for GitHub

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

Jump to bottom

#12897 - add mastodon provider #13293

Merged
merged 11 commits into from Oct 25, 2020
Merged

#12897 - add mastodon provider #13293

merged 11 commits into from Oct 25, 2020

Conversation

divbhasin
Copy link
Contributor

This PR targets #12897. I included the Mastodon provider in oauth2.go and I am looking for feedback on if I have added (and vendored) the module correctly. The diff seems to be very large and I am wondering if that's normal when adding a new module.

@lafriks
Copy link
Member

lafriks commented Oct 24, 2020

You need to add changes also in:
models/oauth2.go
templates/admin/auth/new.tmpl
and also add mastodon icon

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 24, 2020
@techknowlogick
Copy link
Member

Awesome!! Thanks for the PR :)

A few things:

  • Instead of using Mastodon.New, you'll need to use Mastodon.NewCustomisedURL as otherwise it'll default to mastodon.social and users may want to login with other instances
  • You'll need to update some templates as well, to 1. allow user to specify mastodon instance they'd like to use 2. to add it to dropdown
  • Here is an example of a PR that changes similar things: nextcloud oauth #10562

@techknowlogick techknowlogick added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Oct 24, 2020
@techknowlogick techknowlogick added this to the 1.14.0 milestone Oct 24, 2020
@divbhasin
Copy link
Contributor Author

divbhasin commented Oct 24, 2020 •
edited

@techknowlogick I have applied all the suggestions except handling of a custom instance URL. I see that for providers with the custom URL mapping enabled, we have specific endpoints you can customize like the authURL and tokenURL. For mastodon, should we not do any of that and just have the ability to specify a custom instanceURL i.e. something like this:
Screen Shot 2020-10-24 at 3 24 36 PM

I will also have to another field to the CustomURLMapping struct for InstanceURL, correct?

@techknowlogick
Copy link
Member

techknowlogick commented Oct 25, 2020 •
edited

should we not do any of that and just have the ability to specify a custom instanceURL i.e. something like this:

Yup :)

I will also have to another field to the CustomURLMapping struct for InstanceURL, correct?

You could use an existing field (I recommend using AuthURL), but pass it to Mastodon.NewCustomisedURL as instanceURL.

@divbhasin
Copy link
Contributor Author

@techknowlogick unable to add you as reviewer but please take a look. Also, is drone down? The automated check doesn't seem to be running.

techknowlogick
techknowlogick reviewed Oct 25, 2020
View reviewed changes
web_src/js/index.js Outdated Show resolved Hide resolved
@lafriks @techknowlogick
Update web_src/js/index.js
3144e26 
Co-authored-by: techknowlogick <matti@mdranta.net>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 25, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 25, 2020
@codecov-io
Copy link

Codecov Report

Merging #13293 into master will increase coverage by 0.04%.
The diff coverage is 17.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13293      +/-   ##
==========================================
+ Coverage   42.10%   42.14%   +0.04%     
==========================================
  Files         689      689              
  Lines       75721    75771      +50     
==========================================
+ Hits        31881    31933      +52     
- Misses      38610    38621      +11     
+ Partials     5230     5217      -13
Impacted Files Coverage Δ
models/oauth2.go 33.33% <ø> (ø)
services/mirror/mirror.go 17.35% <4.44%> (-1.17%) ⬇️
modules/auth/oauth2/oauth2.go 27.63% <28.57%> (+0.04%) ⬆️
modules/setting/storage.go 89.18% <100.00%> (+16.21%) ⬆️
modules/storage/storage.go 58.18% <100.00%> (ø)
modules/storage/helper.go 22.72% <0.00%> (-31.82%) ⬇️
modules/charset/charset.go 68.53% <0.00%> (-4.50%) ⬇️
services/pull/temp_repo.go 26.59% <0.00%> (-3.20%) ⬇️
models/unit.go 46.57% <0.00%> (-2.74%) ⬇️
services/pull/patch.go 53.97% <0.00%> (-1.71%) ⬇️
... and 21 more

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 f565cf5...878511e. Read the comment docs.

@techknowlogick techknowlogick merged commit 7974b34 into go-gitea:master Oct 25, 2020
@6543 6543 mentioned this pull request Oct 25, 2020
Closed
ivanvc added a commit to ivanvc/gitea that referenced this pull request Oct 27, 2020
@ivanvc
Merge remote-tracking branch 'origin/master' into ensure-user-email-i…
e259244 
…s-stored-in-email-address-table

* origin/master:
  [UI] Hide consecutive additions and removals of the same label (go-gitea#13315)
  [skip ci] Updated translations via Crowdin
  Fix send mail (go-gitea#13312)
  [skip ci] Updated translations via Crowdin
  Deny wrong pull (go-gitea#13308)
  Group Label Changed Comments in timeline (go-gitea#13304)
  [skip ci] Updated translations via Crowdin
  Attempt to handle unready PR in tests (go-gitea#13305)
  go-gitea#12897 - add mastodon provider (go-gitea#13293)
  [skip ci] Updated translations via Crowdin
  Fix Storage mapping (go-gitea#13297)
  Update Mirror IsEmpty status on synchronize (go-gitea#13185)
  Fix bug isEnd detection on getIssues/getPullRequests (go-gitea#13299)
  systemd service: Add commented PATH environment option for Git prefix (go-gitea#13170)
  Sendmail command (go-gitea#13079)
  Various UI and arc-green fixes (go-gitea#13291)
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@techknowlogick techknowlogick techknowlogick approved these changes

@lafriks lafriks lafriks approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@divbhasin @lafriks @techknowlogick @codecov-io @GiteaBot