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

Delete an organisation member #668

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

shhivam
Copy link

@shhivam shhivam commented Nov 2, 2024

Closes: #278

Because

We want to add functionality to delete a member from an organisation

Disclaimer about some unrelated PR changes

  1. The Justfile didn't seem to work anymore because the name of the docker images changed
  2. I have also added /rails/tmp/pids to compose.yml because a persistent "/rails/tmp/pids/server.pid" was giving me pain whenever I restarted docker containers.

Pending things

Writing tests for this feature

Demo

Screen.Recording.2024-11-03.at.03.45.20.mov

Shivam Singhal added 4 commits November 3, 2024 03:33
During development, when I restarted the docker container after the initial start, my rails process was still there and that was causing problems with server startup
Copy link
Member

@kitallis kitallis left a comment

Choose a reason for hiding this comment

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

Mostly looks good, one major comment. I'll review it again once the tests show up.

@@ -11,6 +11,7 @@ x-app: &app
tmpfs:
- /tmp
- /app/tmp/pids
- /rails/tmp/pids
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: This is fine, but remember, docker already synchronizes files from local. So you can technically just remove it from outside the docker container rm /tmp/pid.

Copy link
Author

Choose a reason for hiding this comment

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

I looked into that first but this director wasn't present on my local filesystem and I couldn't check into the server container because by the time I could exec into it, the container would have crashed and stopped.

So, I just added it in tmpfs. Let me know if I should dig more into this so that we don't have to add /rails/tmp/pids to tmpfs

Copy link
Member

@kitallis kitallis Nov 4, 2024

Choose a reason for hiding this comment

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

So / is mounted at rails/ in the docker container. So if you saw an error like this:

web-1       | A server is already running (pid: 1, file: /rails/tmp/pids/server.pid).

That file would be in rm tmp/pids/server.pid in local host.

app/controllers/accounts/memberships_controller.rb Outdated Show resolved Hide resolved
app/helpers/organizations_helper.rb Outdated Show resolved Hide resolved
app/controllers/accounts/memberships_controller.rb Outdated Show resolved Hide resolved
app/views/accounts/organizations/_teams.html.erb Outdated Show resolved Hide resolved
@shhivam shhivam force-pushed the feat/delete-org-member branch from d697990 to 736eb96 Compare November 5, 2024 02:50
@shhivam shhivam force-pushed the feat/delete-org-member branch from 736eb96 to 6f6cd1c Compare November 10, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Shipped
Development

Successfully merging this pull request may close these issues.

2 participants