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

Adding is_manager feild #99

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Adding is_manager feild #99

wants to merge 12 commits into from

Conversation

Sahil590
Copy link

@Sahil590 Sahil590 commented Jan 15, 2025

Description

Adding is_manager field

Fixes #70 and #72 (issue)
I have added the new field to the model

Type of change

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass (eg. python -m pytest)
  • The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
  • Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

@Sahil590 Sahil590 requested review from cc-a, jamesturner246 and AdrianDAlessandro and removed request for cc-a and jamesturner246 January 15, 2025 11:16
Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

Implementation looks good but this seems to include the changes from #98.

@Sahil590
Copy link
Author

Sahil590 commented Jan 15, 2025

My bad, I was making this change while on that branch and switched to this one with the changes. Just fixed it

@Sahil590 Sahil590 linked an issue Jan 15, 2025 that may be closed by this pull request
@Sahil590 Sahil590 requested a review from cc-a January 15, 2025 12:01
@Sahil590
Copy link
Author

Added the logic for the send_group_invite view since it was a small change

Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

Actually... please add a test for the view function change.

@Sahil590 Sahil590 requested a review from cc-a January 16, 2025 13:20
@Sahil590 Sahil590 changed the title is_manager feild Adding is_manager feild Jan 16, 2025
Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

I'm going to be super picky and say that I'd prefer a linear history for the migrations. Rather than merging you can just delete the migrations added in this PR and makemigrations again and you should end up with a single migration.

@Sahil590 Sahil590 requested a review from cc-a January 17, 2025 10:13
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.

Allow group managers to send invites Add is_manager field to GroupMembership model.
2 participants