Skip to content

Conversation

turger
Copy link

@turger turger commented Oct 19, 2019

I tested planner locally and found out that I could't access to admin page to test some functionalities. I didn't find instructions to add myself as and admin locally, so I added these instructions for everyone to use.

Copy link
Contributor

@notapatch notapatch left a comment

Choose a reason for hiding this comment

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

Obviously, brilliant that you are helping on-boarding. 👍

I don't use Docker much because my machine is too slow. I've left a couple of comments that might reduce the instructions needed.

(underline: that I'm a contributor and not a reviewer with commit access)


No such thing as too simple?

Related to this PR: I work in native mode and not docker so I can become admin at the command line (without even using the console):

rails runner "Member.where(name: '<Your first name>').add_role(:admin)"

or

  1. sign up
  2. rails runner "Member.last.add_role(:admin)"

FYI: There's no output it just does it ... so if you want to see anything you should use puts:
rails runner "puts Member.where(name: '<Your first name>')

However, I've not been able to adapt it to Docker. If you can do something similar in Docker ... I'm sure I can, or you as you wish, get a script together, like dbuild, dstart etc, and then the user types dadmin <name | default to last> and hey presto you're an admin.

CONTRIBUTING.md Outdated
* Exit with `exit` twice.
* Then access rails console:
* Run `./bin/drails console`
* Then in the console run: `Member.find(<your-id>).add_role(:admin)`
Copy link
Contributor

Choose a reason for hiding this comment

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

The Guide does indeed use Member ids to make admin. However, I think you can make it a bit shorter miss out trying to find the Member ID and go straight to the console:

Member.where(name: '<Your first name>').add_role(:admin)

Another shortcut:

  1. Sign up to codebar:
  2. Member.last.add_role(:admin)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comments! I tried this with "Member.where(name: 'Taru').add_role(:admin)" but only got error:

/usr/local/bundle/gems/activerecord-4.2.11.1/lib/active_record/relation/delegation.rb:136:in `method_missing': D, [2019-10-20T15:44:22.443021 #1] DEBUG -- :   Member Load (1.1ms)  SELECT "members".* FROM "members" WHERE "members"."name" = $1  [["name", "Taru"]]
undefined method `add_role' for #<Member::ActiveRecord_Relation:0x00000000066e9b18> (NoMethodError)

But the other one Member.last.add_role(:admin) worked like a charm! I will add dadmin-script as you suggested, because I too think that's a much nicer way for solving this. I will also add your instructions for non-docker users to contributing-readme. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

re: Where Oops.

Where solution

where returns, sort of in the loosest way, an array of Members and an array won't know what an add_role method is so you got the "undefined method" exception. A fix is to get the actual member from the sort-of array:

Member.where(name: '<you first name'>).first.add_role(:admin)

Assuming your name is unique.

find_by solution

find_by returns an actual Member and not an array of members. Assuming your the first person named 'Taru' thinks which I reckon there's a decent chance.

Member.find_by(name: '<your first name>').add_role(:admin)

References

find_by
where

Copy link
Author

Choose a reason for hiding this comment

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

Cool, thanks! I edited instructions and dadmin script to use find_by.

CONTRIBUTING.md Outdated
Make sure you have started your app in docker (if using locally, skip the docker-related parts in these instructions) and signed up as a user on your locally running app.
* First get your id:
* Run `docker ps` to get the id of your running postgres container id.
* Run `docker exec -it <your-id-here> psql -U postgres planner_development` where planner_development is the database name.
Copy link
Contributor

Choose a reason for hiding this comment

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

side note: since this is docker compose I think you can use the name of the container which is db .. instead of the id... so something like: docker exec -it db psql -U postgres planner_development

@turger turger force-pushed the add-admin-instructions branch from d5b76ba to b28cf9f Compare October 20, 2019 15:57
@turger
Copy link
Author

turger commented Oct 20, 2019

Made suggested changes, ready for a new check!

@turger turger force-pushed the add-admin-instructions branch from b28cf9f to 811e91a Compare October 20, 2019 18:31
Copy link
Contributor

@notapatch notapatch left a comment

Choose a reason for hiding this comment

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

Looks good to me, but, the actual views that count are the maintainers. They will be around sometimes during the week. I don't know what their schedule is or what their priority is but a readme change and a straight forward script has a good chance!

CONTRIBUTING.md Outdated
5. Run the tests. We only take pull requests with passing tests, and it's great to confirm that you have a clean slate.
5. (Optional) Note that to be able to use the page as an admin, you must first give yourself admin privileges. Make sure you have started your app and signed up as an user on your locally running app, so that you are the last added user.
* Non-docker users can run this on command line: `rails runner "Member.last.add_role(:admin)"`
Copy link
Contributor

Choose a reason for hiding this comment

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

CONTRIBUTING.md Outdated
5. Run the tests. We only take pull requests with passing tests, and it's great to confirm that you have a clean slate.
5. (Optional) Note that to be able to use the page as an admin, you must first give yourself admin privileges. Make sure you have started your app and signed up as an user on your locally running app.
* Non-docker users can run this on command line: `rails runner "Member.find_by(name: '<your first name>').add_role(:admin)"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@matyikriszta matyikriszta Oct 21, 2019

Choose a reason for hiding this comment

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

@turger as @notapatch mentioned can we add the rails runner script to the native installation steps and the docker script to the main readme please and remove it from here? As I mentioned in my general comment this document will be reviewed/rewritten to contain contributing guidelines and not a setup guide.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll move the instructions!

@biggianteye
Copy link
Contributor

biggianteye commented Oct 20, 2019

This is great. I've been looking for how to do this. I think this is a great addition.

Related to this PR: I work in native mode and not docker so I can become admin at the command line (without even using the console):

rails runner "Member.where(name: '<Your first name>').add_role(:admin)"

or

  1. sign up
  2. rails runner "Member.last.add_role(:admin)"

Adding docker-compose run --rm web before those commands is enough to get them running in docker. I've just done the second option like this and it worked perfectly.

docker-compose run --rm web rails runner "Member.last.add_role(:admin)"

Edited to remove something that had already been suggested, but I missed.

bin/dadmin Outdated
Comment on lines 3 to 17
set -e

function cleanup {
# capture exit code
code=$?
echo "cleaning up"

# ignore errors
set +e
docker-compose down

exit $code
}

trap cleanup EXIT
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recommend having this boilerplate taken from the other scripts. It will shut everything down any time you run this script. Probably not what you want to do.

Something like this should be sufficient:

#!/bin/sh

if [ $# -gt 0 ]
then
  docker-compose run --rm web rails runner "Member.find_by(name: '$@').add_role(:admin)"
else
  docker-compose run --rm web rails runner "Member.last.add_role(:admin)"
fi

Copy link
Contributor

@notapatch notapatch Oct 20, 2019

Choose a reason for hiding this comment

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

OOOkay, I get it. Doh! Thanks @biggianteye. The start of the original script clears up the current docker instance? Marvelous.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a tangent, but there are two things happening.

  1. trap cleanup EXIT

The trap keyword allows you to intercept various signals received by the script. This particular line means that when the EXIT signal is received (ie. normal termination), the cleanup function is called.

This minimal example demonstrates this behaviour:

#!/bin/sh

function cleanup
{
    echo "Cleanup function being called"
}

trap cleanup EXIT

echo "Hello world"
[burhan@enceladus ~] {0} $ ./test.sh
Hello world
Cleanup function being called
  1. docker-compose down

This is the important line in the cleanup function. It tells docker to bring down all the containers defined in docker-compose.yml. ie. web (the web server) and db (the database).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks that's really clear. I've only recently worked out how important scripts are for getting work done. Marvellous, a 3 person solution.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, thanks! I made suggested changes to a script and now it's much better.

CONTRIBUTING.md Outdated
5. Run the tests. We only take pull requests with passing tests, and it's great to confirm that you have a clean slate.
5. (Optional) Note that to be able to use the page as an admin, you must first give yourself admin privileges. Make sure you have started your app and signed up as an user on your locally running app.
* Non-docker users can run this on command line: `rails runner "Member.find_by(name: '<your first name>').add_role(:admin)"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@turger this is a minor point but can we suggest people find themselves by email address? I think that's a more reliable piece of information then first name.

Copy link
Author

Choose a reason for hiding this comment

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

Very good point, I'll change name to email!

@matyikriszta
Copy link
Contributor

matyikriszta commented Oct 21, 2019

@turger this is a great addition, thanks for the PR. I left you two small comments. As a more general note, upon further introspection I find it weird we have setup instruction in 3 different places, e.g. the readme, the contributing guide and also the guide on how to set up the app directly to your local environment. I propose we add the docker script to the main README.md, the rails script to the native-installation-instructions.md, and remove the content from CONTRIBUTING.md.

As for CONTRIBUTING.md I will spend some time reviewing and rewriting the content to reflect contribution guidelines (e.g. what to put in a PR, who to ping if there are issues etc.).

@notapatch @biggianteye let me know if that sounds good/you have any feedback.

@turger turger force-pushed the add-admin-instructions branch from 811e91a to d11b3a7 Compare October 22, 2019 18:56
@turger turger force-pushed the add-admin-instructions branch from d11b3a7 to c0ddf74 Compare October 22, 2019 18:58
Copy link
Contributor

@matyikriszta matyikriszta left a comment

Choose a reason for hiding this comment

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

Amazing work @turger, thanks for your contribution.

@matyikriszta matyikriszta merged commit defc76e into codebar:master Oct 23, 2019
@biggianteye
Copy link
Contributor

@notapatch @biggianteye let me know if that sounds good/you have any feedback.

Sorry, I missed this. This patch is wonderful and is a great quality of life improvement for new people to the project. I've already used it a number of times to test some things out. I'm all for more things like this.

As to CONTRIBUTING.md, I'm more than happy to look at any changes that are made to it. Github provides some "anatomy of an open source project" guidance which talks about what to put in different documentation. Things grow organically so it's useful to occasionally take a step back and re-evaluate if things are in the right place.

@turger turger deleted the add-admin-instructions branch November 25, 2019 17:20
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.

4 participants