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

New local environment directions #253

Merged
merged 1 commit into from
Jul 18, 2022
Merged

New local environment directions #253

merged 1 commit into from
Jul 18, 2022

Conversation

okaycj
Copy link
Contributor

@okaycj okaycj commented Jun 21, 2022

Here are the new Django local environment instructions.

@okaycj okaycj requested review from becky-gilbert and mekline June 21, 2022 19:54
@becky-gilbert
Copy link
Contributor

@okaycj FYI I haven't forgotten about reviewing this - I'm just putting together my own notes on the installation process and then will cross-reference those with what you have here. Might take a few days since I'm still consolidating new info and getting interrupted with other things.


brew update && brew upgrade
Before you can begin setting your local environment, we'll need to install a couple of things.
Copy link
Contributor

@becky-gilbert becky-gilbert Jun 30, 2022

Choose a reason for hiding this comment

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

In this section maybe we should add:


.. code-block:: shell
make serve
Copy link
Contributor

Choose a reason for hiding this comment

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

Some commands (like this one, I think) generate a lot of output in the terminal. It might be helpful to add screenshots or text blocks showing this output and what it should look like if it has finished successfully. (If you agree then we could add this later, if you don't want to do it for this PR.)


poetry run create-db
At this point, the services should all be up and running. Direct your browser to https://localhost:8000 to see the local environment. In the future, to start the services you will only need to run "make serve"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
At this point, the services should all be up and running. Direct your browser to https://localhost:8000 to see the local environment. In the future, to start the services you will only need to run "make serve"
At this point, the services should all be up and running. Direct your browser to https://localhost:8000 to see the local environment. In the future, to start the services you will only need to run `make serve`. You will need to keep the terminal window/tab open while using your local server. You can stop the server with COMMAND/CTRL+C or by closing the terminal.

Copy link
Contributor

Choose a reason for hiding this comment

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

And this might be another place to add a screenshot or gif of the browser at localhost:8000.


.. code-block:: shell
Django
Copy link
Contributor

Choose a reason for hiding this comment

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

When I first read through this, I thought this and subsequent sections were still part of the initial set-up. So could we make that distinction clearer by replacing the Django heading here with something like "Useful Commands", and then nested under that put subheadings for "Django", "Rabbitmq", "Postgres", "Authentication" and "Handling Video"?

I also think it would be helpful to have a short introductory sentence at the start of each of these subsections, indicating what this part of the system does. E.g. for Django: "This is the Python-based web framework that is used to create the Lookit website (everything except for running the studies themselves)", for Postgres: "This is the database that stores information that the Lookit website needs, such as user accounts, study information <etc.>". This might be helpful for users who encounter some kind of problem after initial set up but don't know which framework/service to focus on.

Rabbitmq
~~~~~~~~

The broker should come up with the rest of the Docker services. If you get a Celery worker error due to permissions, you can run the following command to resolve the issue and restart the worker service:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be useful to say what the Celery worker error looks like, e.g. with a screenshot or text.


.. code-block:: shell

poetry run invoke celery-service
cat /location/of/sql/file | make dbpipe


Authentication
Copy link
Contributor

Choose a reason for hiding this comment

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

This section says "you will need to log in using the superuser you created earlier using manage.py", which I think is referencing make superuser on line 104, but I don't see any references to "manage.py" on this page, so that might be confusing. Is that left over from an earlier version?

@@ -208,22 +167,3 @@ during testing:
Then, based on the the assigned URL, you will need to manually edit the webhook on the
Copy link
Contributor

Choose a reason for hiding this comment

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

(This applies to the start of the Handling Video section, I'm just not able to comment in the right place...)
I think it would be helpful to explain in simpler terms when this step is needed. E.g.
"You will be able to run internal Lookit studies through your development server, but this additional step is needed if you want to generate and save video recordings. The project includes an incoming webhook handler...".

@mekline mekline merged commit 3411458 into develop Jul 18, 2022
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.

3 participants