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

contrib: minor CERN plugin fix #64

Merged
merged 2 commits into from
Jul 1, 2016
Merged

contrib: minor CERN plugin fix #64

merged 2 commits into from
Jul 1, 2016

Conversation

omelkonian
Copy link
Contributor

@omelkonian omelkonian commented Jun 23, 2016

  • Fixes an issue where g.identity.provides was populated with only the
    Cern groups. As access rights can be assigned to single users(i.e. emails),
    the user's e-mail must also be included in the g.identity.provides.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage increased (+0.02%) to 85.314% when pulling c017c59 on omelkonian:master into a1779d8 on inveniosoftware:master.

@jirikuncar
Copy link
Member

@omelkonian please amend commit message (s/Cern/CERN/) and reformat the bullet points (kwalitee check message).

@jirikuncar jirikuncar added this to the v1.0.0 milestone Jun 23, 2016
@@ -236,7 +236,8 @@ def account_setup(remote, token, resp):
oauth_link_external_id(user, dict(id=external_id, method="cern"))
Copy link
Member

@jirikuncar jirikuncar Jun 23, 2016

Choose a reason for hiding this comment

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

Could you also unify usage of quotes to ' (if you start please check the whole file - """ docstrings, ' everything else). Thanks

@jirikuncar jirikuncar changed the title contrib: minor Cern plugin fix contrib: minor CERN plugin fix Jun 23, 2016
@omelkonian
Copy link
Contributor Author

Although I cannot find any occurence of 'Cern'.

@jirikuncar
Copy link
Member

@omelkonian look to the commit message ...

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+0.02%) to 85.314% when pulling 6cb4c58 on omelkonian:master into be2e979 on inveniosoftware:master.

@jirikuncar
Copy link
Member

@omelkonian please remove merge commit.

@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage increased (+0.02%) to 85.314% when pulling 4d8dd17 on omelkonian:master into be2e979 on inveniosoftware:master.

@jirikuncar jirikuncar self-assigned this Jun 27, 2016
@jirikuncar
Copy link
Member

@omelkonian I have checked the diff again and I see that you have replaced the quotes everywhere. It's ok, but it should be in separate commit. Also make sure that you write CERN and not Cern.

@jirikuncar
Copy link
Member

WDYT about s/contrib: strings' formatting/global: quotes usage unification/?


groups = fetch_groups(res['Group'])
session['identity.cern_provides'] = [RoleNeed(group) for group in groups]
provides = [UserNeed(user.email)] + \
[RoleNeed(group + '@cern.ch') for group in groups]
Copy link
Member

Choose a reason for hiding this comment

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

can you use '{0}@cern.ch'.format(group) instead? you know ... safety first ;-)

@jirikuncar
Copy link
Member

@omelkonian please use labels (FIX, NOTE) only when the message is important for readers of release notes. In the case of first commit you can remove the FIX label as it is just styling (* FIX Converts strings to be more uniform ...) and if you would really like to announce such change then label BETTER [1] is more appropriate.

[1] http://invenio.readthedocs.io/en/latest/technology/git.html#r2-remarks-on-commit-log-messages

@egabancho
Copy link
Member

Besides @jirikuncar's comment LGTM

@jirikuncar jirikuncar removed their assignment Jul 1, 2016
@jirikuncar
Copy link
Member

@omelkonian please keep the branch up-to-date with the master branch and have a look to unresolved comments.

* Converts strings to be more uniform (i.e. `"""` for docstrings and
  `'` for everything else).

Signed-off-by: Orestis Melkonian <melkon.or@gmail.com>
* Fixes an issue where `g.identity.provides` was populated with
  only the Cern groups. As access rights can be assigned to single
  users (i.e. emails), the user's e-mail must also be included in the
  `g.identity.provides`.

* NOTE Appends '@cern.ch' to every CERN group, for uniform formatting.

Signed-off-by: Orestis Melkonian <melkon.or@gmail.com>
@jirikuncar jirikuncar merged commit b8e7f0c into inveniosoftware:master Jul 1, 2016
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.

None yet

4 participants