Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

Bug 1273037 - Move from Personal to Google Auth #15

Merged
merged 3 commits into from
Sep 23, 2016
Merged

Bug 1273037 - Move from Personal to Google Auth #15

merged 3 commits into from
Sep 23, 2016

Conversation

jezdez
Copy link
Contributor

@jezdez jezdez commented Sep 16, 2016

This change is Reviewable

@mozilla-autolander-deprecated

Autolander could not find a bug number in your pull request title. All pull requests should be in the format of: Bug [number] - [description].

@jezdez jezdez changed the title Move from Personal to Google Auth Bug 1273037 - Move from Personal to Google Auth Sep 16, 2016
@maurodoglio
Copy link
Contributor

Please ping/mention me when this is ready for a review 😺

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 77.278% when pulling a6239b2 on allauth into cff0afc on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 76.775% when pulling 5af8d85 on allauth into cff0afc on master.

@jezdez
Copy link
Contributor Author

jezdez commented Sep 20, 2016

@maurodoglio Okay, this seems good to review, the allauth templates that we're using right now don't match the style and may require some rewording here and there, I'll get to that tomorrow.

When you test this, you won't have to enter credentials separately, I've created a google client ID limited to localhost and it's being automatically added to the database (where allauth stores its creds) during the migrations.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 76.803% when pulling 9d66b87 on allauth into cff0afc on master.

"""
if len(accounts) == 1:
raise forms.ValidationError(
'At least one account needs to be connected to your'
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ to your//

Copy link
Contributor

@maurodoglio maurodoglio 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 to me, there is only a small nit. It's probably worth to squash the commits before merging it.

@@ -27,7 +27,13 @@
<li class="dropdown">
<a href="#" class="dropdown-toggle" data-toggle="dropdown" role="button" aria-haspopup="true" aria-expanded="false">{{ request.user.email }} <span class="caret"></span></a>
<ul class="dropdown-menu">
<li>{% browserid_logout text='Log Out' next='/login' %}</li>
{% if request.user.is_authenticated() %}
<li><a href="{{ url('account_email') }}">Emails</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a template tag? Maybe it's a leftover from the jinja template?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually nevermind, I noticed you fixed it in one of the commits below

</ul>
</div>
<div class="col-md-6">
<h2>Links & resources</h2>
Copy link

Choose a reason for hiding this comment

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

doesn't this need to be &amp;

defaults={
'name': 'Google',
'client_id': '559124132137-h9jjj9jm7762uq7u2mn8h8dki7re28tv.apps.googleusercontent.com',
'secret': '1iNc8DKhNXoey8jLs4y-E6MN',
Copy link

Choose a reason for hiding this comment

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

???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced it with a make task and management command, thanks for the heads-up

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 76.803% when pulling d8b5e9d6a53934391cd7a95498b7f7f50a06edeb on allauth into 94c6bf4 on master.

Also:

- Rename some classes to just Atmo
- Remove unused django-browserid backend
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.04%) to 75.621% when pulling cbf7fcd on allauth into 94c6bf4 on master.

@maurodoglio
Copy link
Contributor

:shipit:

@jezdez jezdez merged commit e49da86 into master Sep 23, 2016
@jezdez jezdez deleted the allauth branch September 23, 2016 07:50
washort pushed a commit to washort/telemetry-analysis-service that referenced this pull request Aug 15, 2017
jezdez pushed a commit that referenced this pull request Jan 17, 2019
Bumps [notebookjs](https://github.com/jsvine/notebookjs) from 0.3.0 to 0.3.2.
<details>
<summary>Commits</summary>

- [`993e983`](jsvine/notebookjs@993e983) Fix `var lang =` bug and bump to v0.3.2
- [`cd4dcdf`](jsvine/notebookjs@cd4dcdf) Bump to v0.3.1
- [`cfa4831`](jsvine/notebookjs@cfa4831) Merge pull request [#15](https://github-redirect.dependabot.com/jsvine/notebookjs/issues/15) from bradhowes/master
- [`d137a66`](jsvine/notebookjs@d137a66) Add support for a custom highlighter function.
- [`587f7ef`](jsvine/notebookjs@587f7ef) Add thanks to [**jithurjacob**](https://github.com/jithurjacob) in README.md
- See full diff in [compare view](jsvine/notebookjs@v0.3.0...v0.3.2)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=notebookjs&package-manager=npm_and_yarn&previous-version=0.3.0&new-version=0.3.2)](https://dependabot.com/compatibility-score.html?dependency-name=notebookjs&package-manager=npm_and_yarn&previous-version=0.3.0&new-version=0.3.2)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

---

**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants