Since this file is still committed, if changes are made, the changes can still be committed. It is better practice to provide an example file that someone can copy into the "correct" file for the app to read. For example: fas-admin-config.json.example Then, this line in the gitignore file will work correctly.

This is a nitpick, but for what we present to the user, I think we should stick to "FAS username" instead of "FAS-ID". I believe it will better recognized if written like this.

No such FAS-ID exsist => No such FAS username exists.

FAS-ID => FAS username

I was confused initially because I tried searching for someone's email address to look-up their name. I think we should reword this, maybe something like Search for a FAS username. What do you think?

Minor typo. accountExsist => account_exists

I suggest naming this file fas-admin-config.json.example so it is not accidentally checked into git by mistake.

@alishapapun Excellent work with this PR! :tada: :tada: This is awesome to see. :smile:

I left some in-line comments above, but they are mostly small details. There is one issue I ran into though. When I tested the application with no FAS credentials, there isn't error handling for when there are incorrect credentials provided:

web_1       | [19/Mar/2019 02:04:57] "GET /send/search?fasid=jflo HTTP/1.1" 301 0
web_1       | Internal Server Error: /send/search/
web_1       | Traceback (most recent call last):
web_1       |   File "/usr/local/lib/python3.6/site-packages/django/core/handlers/exception.py", line 35, in inner
web_1       |     response = get_response(request)
web_1       |   File "/usr/local/lib/python3.6/site-packages/django/core/handlers/base.py", line 128, in _get_response
web_1       |     response = self.process_exception_by_middleware(e, request)
web_1       |   File "/usr/local/lib/python3.6/site-packages/django/core/handlers/base.py", line 126, in _get_response
web_1       |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
web_1       |   File "/usr/local/lib/python3.6/contextlib.py", line 52, in inner
web_1       |     return func(*args, **kwds)
web_1       |   File "/app/happinesspackets/messaging/views.py", line 213, in fasidCheck
web_1       |     person = fas.person_by_username(fasid)
web_1       |   File "/usr/local/lib/python3.6/site-packages/fedora/client/fas2.py", line 445, in person_by_username
web_1       |     req_params=params)
web_1       |   File "/usr/local/lib/python3.6/site-packages/fedora/client/baseclient.py", line 367, in send_request
web_1       |     auth_params=auth_params, retries=retries, timeout=timeout)
web_1       |   File "/usr/local/lib/python3.6/site-packages/fedora/client/proxyclient.py", line 462, in send_request
web_1       |     'Unable to log into server.  Invalid'
web_1       | fedora.client.AuthError: Unable to log into server.  Invalid authentication tokens.  Send new username and password

It would be helpful to make it more clear to the user if there is a configuration error and they should be blocked from making a new search while one is already in progress. Do you think you could add some error handling for invalid authentication credentials in the back-end and front-end?

@alishapapun Excellent work with this PR! 🎉 🎉 This is awesome to see. 😄
I left some in-line comments above, but they are mostly small details. There is one issue I ran into though. When I tested the application with no FAS credentials, there isn't error handling for when there are incorrect credentials provided:
web_1 | [19/Mar/2019 02:04:57] "GET /send/search?fasid=jflo HTTP/1.1" 301 0
web_1 | Internal Server Error: /send/search/
web_1 | Traceback (most recent call last):
web_1 | File "/usr/local/lib/python3.6/site-packages/django/core/handlers/exception.py", line 35, in inner
web_1 | response = get_response(request)
web_1 | File "/usr/local/lib/python3.6/site-packages/django/core/handlers/base.py", line 128, in _get_response
web_1 | response = self.process_exception_by_middleware(e, request)
web_1 | File "/usr/local/lib/python3.6/site-packages/django/core/handlers/base.py", line 126, in _get_response
web_1 | response = wrapped_callback(request, callback_args, callback_kwargs)
web_1 | File "/usr/local/lib/python3.6/contextlib.py", line 52, in inner
web_1 | return func(args, **kwds)
web_1 | File "/app/happinesspackets/messaging/views.py", line 213, in fasidCheck
web_1 | person = fas.person_by_username(fasid)
web_1 | File "/usr/local/lib/python3.6/site-packages/fedora/client/fas2.py", line 445, in person_by_username
web_1 | req_params=params)
web_1 | File "/usr/local/lib/python3.6/site-packages/fedora/client/baseclient.py", line 367, in send_request
web_1 | auth_params=auth_params, retries=retries, timeout=timeout)
web_1 | File "/usr/local/lib/python3.6/site-packages/fedora/client/proxyclient.py", line 462, in send_request
web_1 | 'Unable to log into server. Invalid'
web_1 | fedora.client.AuthError: Unable to log into server. Invalid authentication tokens. Send new username and password

This error occurred because you have not changed the ADMIN_USERNAME and ADMIN_PASSWORD in the fas-admin-details.json to any authenticated username and email ( Invalid authentication tokens refers to the wrong credentials passed to the api of FAS which is read from the fas-admin-details.json ). After you provide proper username and password, there is proper error handling by showing an error message of username does not exsist.

It would be helpful to make it more clear to the user if there is a configuration error and they should be blocked from making a new search while one is already in progress. Do you think you could add some error handling for invalid authentication credentials in the back-end and front-end?

I have made the required changes in the project. Could you please review the changes ? If everything is all right I would just squash my commits into one. Thanks :)

I have made the required changes in the project. Could you please review the changes ? If everything is all right I would just squash my commits into one. Thanks :)

I had a chance to test this now. The changes are great! Thanks for getting on this so quickly.

I think this is almost ready to merge. I'm requesting two additions and two new tickets.

1. :heavy_plus_sign: Addition: On an instance with an incorrect/missing FAS username/password, when someone makes a search, some sort of one-line error message should go out on the console too.
2. :heavy_plus_sign: Addition: Quick note in developer environment guide about copying fas-admin-details.json.example to fas-admin-details.json (note you don't need to update the Windows setup guide, see this comment for why).
3. :ticket: New ticket: Add a search button in case pressing Return is not intuitive to user (see screenshot mockup below)
4. :ticket: New ticket: Predict usernames with auto-completion in popup dialog while typing in FAS username search bar

Moving 3 and 4 into new tickets keeps future improvements in mind and lets us move forward with merging this. If you can add error logging to the console and a note in the development environment setup guide, this is ready to merge! :tada:

Thanks again for your work on this PR. :smile:

Hey @jflory7 ! Sorry for piling up bunch of commit message here. I ran into some problem and one problem lined to another while solving the merge conflict. But finally sorted it out. I made the changed that you asked and cleared up the merged conflict that was taking place. It would be great if you could merge the code after testing it ! Thanks :)

Shall I squash the commits or its fine? (If squashing is required, which particular commits you want to squash) ? @jflory7

It would be great if you could merge the code after testing it ! Thanks :)

Thanks @alishapapun! :thumbsup: I just tested it but I wasn't able to verify the console log message. I hoped a message would appear in the docker-compose console for the web container. When I made a search with invalid FAS credentials entered, I only saw this output in console:

web_1 | [22/Mar/2019 00:50:45] "GET /send/search/?fasid=alishapapun HTTP/1.1" 200 128

I was hoping to see an error message to indicate why the search failed (i.e. the FAS login credentials are incorrect).

Shall I squash the commits or its fine? (If squashing is required, which particular commits you want to squash) ? @jflory7

My preference is to squash commits at the end right before merging (it makes it easier to review between giving feedback). When it is ready to merge, squash the commits down to one or two meaningful commits. :smile:

It would be great if you could merge the code after testing it ! Thanks :)

Thanks @alishapapun! 👍 I just tested it but I wasn't able to verify the console log message. I hoped a message would appear in the docker-compose console for the web container. When I made a search with invalid FAS credentials entered, I only saw this output in console:
web_1 | [22/Mar/2019 00:50:45] "GET /send/search/?fasid=alishapapun HTTP/1.1" 200 128
I was hoping to see an error message to indicate why the search failed (i.e. the FAS login credentials are incorrect).

Shall I squash the commits or its fine? (If squashing is required, which particular commits you want to squash) ? @jflory7

My preference is to squash commits at the end right before merging (it makes it easier to review between giving feedback). When it is ready to merge, squash the commits down to one or two meaningful commits. 😄

Made the changes you asked for @jflory7 . :)

Made the changes you asked for @jflory7 . :)

Super! :thumbsup: I tested this and it worked as expected. This is ready to merge! :tada:

Please squash your commits down to 1 or 2 meaningful commits and then I'll merge. :smile: When you have time, don't forget about the two new tickets I mentioned here.

Made the changes you asked for @jflory7 . :)

Super! 👍 I tested this and it worked as expected. This is ready to merge! 🎉
Please squash your commits down to 1 or 2 meaningful commits and then I'll merge. 😄 When you have time, don't forget about the two new tickets I mentioned here.

Hey @jflory7 ! Squashed my commits to one. And yes I remember these two tickets. Will surely proceed with it :thumbsup:

@alishapapun Awesome! We're all set here. Merging! :clapper:

@alishapapun Awesome! We're all set here. Merging! 🎬

The best feeling ever! :heart:

🎫 New ticket: Add a search button in case pressing Return is not intuitive to user (see screenshot mockup below)
🎫 New ticket: Predict usernames with auto-completion in popup dialog while typing in FAS username search bar

Hi @alishapapun, did you have a chance to file these two issues? Let me know if you want to do this or if you prefer for me to file them. I want to make sure we keep track of these two tasks. :smiley: