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

Add urls for change_password modal and login modal. #154

Merged

Conversation

Withington
Copy link
Collaborator

Add the ability to get to the Change Password screen directly from
https://......../#change_password
useful for when the admin has given a user a new password and wants to direct the user to immediately change their password.

Also added url to go directly to login screen
https://......./#login

Needs improvement -
This works fine if your browser tab is not already on Phenopolis. However, if your tab is already open at Phenopolis home screen, then you need to enter the url https://......../#change_password and hit refresh in order to get the modal to pop up.

@Withington Withington requested a review from IsmailM June 13, 2017 16:51
@coveralls
Copy link

Coverage Status

Coverage remained the same at 24.093% when pulling d9e45d0 on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@IsmailM
Copy link
Member

IsmailM commented Jun 13, 2017

Thanks for this 😄

With regards to your point, I think this is good enough tbh - especially since most users will only come across those links via a clickable URL in an email.

Alternatively, the proper fix would be to create another two endpoints - /login and /change_password with the method set to ['GET'] where you simply redirect the correct URL - i.e. https://.../#login or https://.../#change_password ...

@@ -67,6 +67,13 @@ if (!PP) {
});

$(document).ready(function () {
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to have a $(document).ready() here? It should work without?

The PP.initLogin() function is called from the html - see here...

Note - $(document).ready() will be executed once the document is loaded. $(function(){...}); (what we have in the html) is a shortcut for $(document).ready() and does the exact same thing. On the other hand, (function(){...})(); will be executed as soon as it is encountered in the Javascript (i.e. the PP JS module) - basically invoking itself.

AS such there shouldn't be any need have another document.ready function.

Also, if possible move the indexOf lines into a separate function and call it from the initLogin or directly from the html.

I typically prefer to just define functions in the JS library files and then have a single on.ready function in the HTML where I call the necessary functions...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I've made those changes.

@Withington
Copy link
Collaborator Author

Added https://......../change_password and https://......../login. These do refresh the browser when called.

Removed $(document).ready() and put the functionality in to initLogin() instead.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 24.522% when pulling 5c0d488 on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 24.122% when pulling fd4536f on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 24.122% when pulling fd4536f on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 24.122% when pulling fd4536f on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 24.122% when pulling 65b1cb9 on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 24.122% when pulling 65b1cb9 on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@@ -180,6 +180,11 @@ def login():
return jsonify(success="Authenticated"), 200

#
@app.route('/login', methods=['GET'])
def login_form():
return redirect('/#login')
Copy link
Member

Choose a reason for hiding this comment

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

So we need an if statement here and below as the one here: https://github.com/phenopolis/phenopolis/blob/master/views/__init__.py#L187

Copy link
Member

Choose a reason for hiding this comment

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

tbh not sure why that if statement is there but I suspect that it's there because redirecting to the home page caused some issues on the server.

Copy link
Member

Choose a reason for hiding this comment

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

After that I'm happy with this PR 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well spotted. Done, thanks.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 24.095% when pulling 5d97c30 on Withington:change_password_url into 1ccd945 on phenopolis:change_password_url.

@IsmailM IsmailM merged commit b5afa6c into phenopolis:change_password_url Jun 14, 2017
IsmailM added a commit that referenced this pull request Jun 14, 2017
* Add urls for login modal and change_password modal.

* Added new url endpoints for change_password and login.

* Added 'Search Phenopolis' button that becomes visible after a successful password change.

* Added 'Search Phenopolis' button that becomes visible after a successful password change.

* Added 'Search Phenopolis' button that becomes visible after a successful password change.

* Added 'Search Phenopolis' button that becomes visible after a successful password change.

* Added 'Search Phenopolis' button that becomes visible after a successful password change.

* Added 'Search Phenopolis' button that becomes visible after a successful password change.
@Withington Withington deleted the change_password_url branch June 18, 2017 16:25
IsmailM added a commit that referenced this pull request Jul 30, 2017
)"

Alternative logic is used in the dev1 branch - so reverting this
This allows for the dev1 branch to be merged easily...

This reverts commit 0a2cff1.
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

3 participants