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

global: researcher profiles #1249

Closed
wants to merge 3 commits into from
Closed

global: researcher profiles #1249

wants to merge 3 commits into from

Conversation

jainaman224
Copy link
Contributor

No description provided.

@jainaman224 jainaman224 mentioned this pull request Aug 4, 2017
Closed
@slint slint changed the base branch from master to researcher_profile August 4, 2017 12:46
@jainaman224 jainaman224 force-pushed the researcher_profile branch 10 times, most recently from 6727684 to 15f6ddb Compare August 10, 2017 20:32
@jainaman224 jainaman224 force-pushed the researcher_profile branch 6 times, most recently from d95d634 to 6fcf26f Compare August 16, 2017 17:58
@lnielsen
Copy link
Member

@slint @jainaman224 Can we take a review of the status in the coming days? @jainaman224 Would you mind preparing some screenshots with the current status of the pages.

Also, we need to have inveniosoftware/invenio-userprofiles#85 finished up, otherwise the deployment is going to be held up by not having it merged. Currently the PR is missing:

Invenio-UserProfiles is already in beta now prior to Invenio v3 release, so unfortunately the bar for having the PR merged is higher than usual.

@lnielsen lnielsen changed the title Researcher profile global: researcher profiles Aug 16, 2017
@jainaman224
Copy link
Contributor Author

@lnielsen Updated screenshots of pages are at #1212. I will add more of them with github and orcid.
I think for invenio-userprofiles tests are already. Still, if you have any other testing in mind. Please tell me.
I will be added documentation by weekend.

slint
slint reviewed Aug 19, 2017
View reviewed changes
zenodo/modules/profiles/views.py Outdated
orcid_id = account.extra_data.get('orcid')

email = user.email
default = "https://ideaclicks.in/userimages/default_user.jpg"
Copy link
Member

Choose a reason for hiding this comment

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

It's better to have a default image locally and served under /static/default_avatar.jpg, or even better use the one Gravatar provides by default

zenodo/modules/profiles/api.py Outdated
def __init__(self, user_id=None, orcid_id=None, **kwargs):
"""Constructor function."""
if user_id is not None:
query_string = 'owners:' + str(user_id)
Copy link
Member

Choose a reason for hiding this comment

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

This still works but it might be better to use something more programmatic like Q('term', **{'owners': user_id}) to avoid this string concatenation/building.

zenodo/modules/profiles/api.py Outdated
else:
query_string = ''
self.Meta.default_filter = Q('query_string', query=query_string)
super(ResearcherRecordsSearch, self).__init__(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Generally for the search class you should also take into account the versioning "filters" that are applied even for regular results. We can discuss more over chat what record versions mean and how they affect search results.

zenodo/modules/profiles/ext.py Outdated
def init_config(self, app):
"""Flask application initialization."""
for k in dir(config):
if k.startswith("PROFILES_"):
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use ZENODO_PROFILES_ as a prefix in order to avoid collision with invenio-userprofiles and be more explicit about the extension

zenodo/modules/profiles/templates/zenodo_profiles/orcid_profile.html
headers: {'Accept': 'application/json'},
contentType: 'application/json',
dataType: 'json',
success: function(result){
Copy link
Member

Choose a reason for hiding this comment

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

It's better to extract this logic into some separate function, since now it looks pretty complex. There are also some utility functions available I believe that might simplify many parts of the code (eg. Lodash's _.map for extracting deeply nested values from response objects).

zenodo/modules/profiles/views.py
)


@blueprint.route(
Copy link
Member

Choose a reason for hiding this comment

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

You can move this view handler below the base /profile/<int:owner_id> definition, since it's basically a "sub-resource".

zenodo/modules/profiles/views.py Outdated
default = "https://ideaclicks.in/userimages/default_user.jpg"
size = 400

gravatar_url = 'https://www.gravatar.com/avatar/' + hashlib.md5(
Copy link
Member

Choose a reason for hiding this comment

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

It might be more readable to extract this as a uitlity function in utils.py.

zenodo/modules/profiles/views.py Outdated
db.session.add(researcher_profile)

# Update email
if current_app.config['USERPROFILES_EMAIL_ENABLED'] and \
Copy link
Member

Choose a reason for hiding this comment

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

I think this clause is not required since Zenodo profiles have email enabled in config

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 think we can optimise here as well?

zenodo/modules/theme/static/scss/profiles.scss Outdated
}

#researcher-profile, #orcid-profile{
@media screen and (max-width: 768px) {
Copy link
Member

Choose a reason for hiding this comment

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

If possible, extending some existing Bootstrap SASS class might be more clear, since manually handling CSS media queries might become tricky (and is already handled pretty well on Bootstrap's side)

zenodo/modules/utils/files.py Outdated
@@ -0,0 +1,42 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Some thing went wrong with rebasing here (this file shouldn't be part of this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, It is the part of your commit as zenodo/researcher_profile branch is behind master.

@jainaman224
global: profile page
ae6948a 
* Basic Layout.
* Support with angular module.

Signed-off-by: Aman Jain <jainaman224@gmail.com>
@jainaman224
docs: sphinx fix
128dffe 
Signed-off-by: Aman Jain <jainaman224@gmail.com>
@jainaman224 jainaman224 force-pushed the researcher_profile branch 2 times, most recently from 014dd56 to 73b3bc3 Compare August 19, 2017 20:16
@jainaman224 jainaman224 force-pushed the researcher_profile branch 4 times, most recently from 9c05470 to 881fec1 Compare August 23, 2017 16:33
@jainaman224
global: researcher profile
6a00e96 
Signed-off-by: Aman Jain <jainaman224@gmail.com>
@krzysztof krzysztof mentioned this pull request Jan 22, 2018
Open
@slint
Copy link
Member

slint commented Aug 9, 2018

Rebased and merged manually

@slint slint closed this Aug 9, 2018
@jainaman224
Copy link
Contributor Author

Thank you

@ccamara
Copy link

ccamara commented Mar 11, 2020

Hello, I see that this PR mas merged long time ago, but haven't been able to figure out how to create a researcher profile. Can anyone update on the status of this long-awaited feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slint slint slint left review comments

Assignees

@slint slint

@jainaman224 jainaman224

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jainaman224 @lnielsen @slint @ccamara