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

contrib: removal of dependency on Invenio-Groups #19

Merged
merged 2 commits into from
Dec 8, 2015
Merged

contrib: removal of dependency on Invenio-Groups #19

merged 2 commits into from
Dec 8, 2015

Conversation

JavierDelgadoFernandez
Copy link
Contributor

Signed-off-by: Javier Delgado javier.delgado.fernandez@cern.ch

@jirikuncar jirikuncar added this to the v1.0.0 milestone Dec 1, 2015
JavierDelgadoFernandez
JavierDelgadoFernandez reviewed Dec 1, 2015
View reviewed changes
invenio_oauthclient/contrib/cern.py

db.session.add(user)
current_user.reload()
res['groups'] = fetch_groups(res['Group'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really needed? Why is necessary to parse the groups and remove some of them?

Copy link
Member

Choose a reason for hiding this comment

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

Because some of them should not be managed by Invenio.

@jirikuncar
Copy link
Member

IMHO we can create send a general signal in https://github.com/inveniosoftware/invenio-oauthclient/blob/master/invenio_oauthclient/handlers.py#L259-L266 with remote.name and resp.

lnielsen
lnielsen reviewed Dec 1, 2015
View reviewed changes
invenio_oauthclient/contrib/cern.py
@@ -159,6 +160,9 @@
REMOTE_APP_RESOURCE_SCHEMA = "http://schemas.xmlsoap.org/claims/"


oauth_cern_response_received = signal('oauth-cern-response-received')
Copy link
Member

Choose a reason for hiding this comment

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

the signal should be defined in a signals.py file.

Also it's better to define a generic signal oauth-reposnse-received and use the remote app as sender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jirikuncar From handlers.py there is no access to get all the resources of the user. Should the methods account_info and account_setup return a dictionary with all the fields?

Copy link
Member

Choose a reason for hiding this comment

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

please remove this signal

Copy link
Member

Choose a reason for hiding this comment

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

@JavierDelgadoFernandez please remove this signal. It is not needed anymore.

@jirikuncar
Copy link
Member

Please cherry-pick from https://github.com/jirikuncar/invenio-oauthclient/tree/signals

@jirikuncar @JavierDelgadoFernandez
signals: initial implementation
c15544c 
Signed-off-by: Jiri Kuncar <jiri.kuncar@cern.ch>
@JavierDelgadoFernandez
Copy link
Contributor Author

@jirikuncar Ping.

jirikuncar
jirikuncar reviewed Dec 7, 2015
View reviewed changes
invenio_oauthclient/handlers.py
handlers['setup'](token, response)
account_setup = handlers['setup'](token, response)
account_setup_received.send(
remote, response=response, account_setup=account_setup
Copy link
Member

Choose a reason for hiding this comment

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

you might want to pass also token

@JavierDelgadoFernandez
Copy link
Contributor Author

@jirikuncar Done.

jirikuncar
jirikuncar reviewed Dec 7, 2015
View reviewed changes
invenio_oauthclient/contrib/orcid.py
@@ -119,9 +120,13 @@
))


oauth_orcid_response_received = signal('oauth-orcid-response-received')
Copy link
Member

Choose a reason for hiding this comment

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

remove this signal

@JavierDelgadoFernandez
Copy link
Contributor Author

@jirikuncar Done.

@JavierDelgadoFernandez
Copy link
Contributor Author

@jirikuncar Done.

jirikuncar
jirikuncar reviewed Dec 8, 2015
View reviewed changes
invenio_oauthclient/contrib/cern.py

db.session.add(user)
current_user.reload()
return None
Copy link
Member

Choose a reason for hiding this comment

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

pass

@jirikuncar
Copy link
Member

  • please include signals.py in docs

Then we can :shipit:

@JavierDelgadoFernandez
contrib: removal of dependency on Invenio-Groups
1bc2fd2 
* Removes the dependency on Invenio-Groups reducing the number of fields
  parsed inside CERN or ORCID modules and sending the response to a
  subscribable signal. (closes inveniosoftware#18)

Signed-off-by: Javier Delgado <javier.delgado.fernandez@cern.ch>
@JavierDelgadoFernandez
Copy link
Contributor Author

@jirikuncar Done.

@jirikuncar jirikuncar mentioned this pull request Dec 8, 2015
Closed
@jirikuncar jirikuncar merged commit 1bc2fd2 into inveniosoftware:master Dec 8, 2015
@jirikuncar jirikuncar mentioned this pull request Dec 8, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jirikuncar jirikuncar

Labels
Type: enhancement
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.

contrib: remove strong dependency on Invenio-Groups
3 participants
@JavierDelgadoFernandez @jirikuncar @lnielsen