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.

Sign up for GitHub

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

Jump to bottom

Create table user status type 35 #427

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Create table user status type 35 #427

merged 1 commit into from
Nov 13, 2024

Conversation

del9ra
Copy link
Member

@del9ra del9ra commented Oct 28, 2024

Fixes #35

What changes did you make?

  • created a model
  • added seed data
  • tested API endpoints and model

Why did you make the changes (we will use this info to test)?

  • to update a shared data store across hackforla.org, vrms, civictechjobs, and tables project

dmartin4820
dmartin4820 requested changes Oct 29, 2024
View reviewed changes
Copy link
Member

@dmartin4820 dmartin4820 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some minor comments. I switched to your branch in my local peopledepot git area, and I ran

1. ./scripts/buildrun.sh
2. ./scripts/test.sh

without issue.

Other than the few comments, it looks good to me 👍 .

app/core/admin.py Outdated


@admin.register(UserStatusType)
class UserStatusType(admin.ModelAdmin):
Copy link
Member

Choose a reason for hiding this comment

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

The class name should end with "Admin" as shown in the docs here.

app/core/models.py
@@ -426,3 +426,11 @@ class SocMajor(AbstractBaseModel):

def __str__(self):
return self.title


class UserStatusType(AbstractBaseModel):
Copy link
Member

Choose a reason for hiding this comment

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

Now that this model is going to be implemented, we should uncomment the one-to-many relationship here, according to the tool tip under here.

fyliu
fyliu requested changes Oct 31, 2024
View reviewed changes
Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

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

Great work!
I just have a couple of small things.

app/core/tests/test_api.py Outdated
@@ -11,6 +11,7 @@

USER_PERMISSIONS_URL = reverse("user-permission-list")
ME_URL = reverse("my_profile")
USER_STATUS_TYPE_URL = reverse("user-status-type-list")
Copy link
Member

Choose a reason for hiding this comment

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

This should be USER_STATUS_TYPES_URL since it's the url for the list, which contains multiple models.

I see there's many of these lines that got through the PR process with the singular names. Will have to make an issue to to correct those.

app/data/migrations/0007_userstatustype_seed.py Outdated
]
for uuid, name, description in items:
UserStatusType.objects.create(uuid=uuid, name=name, description=description)
pass
Copy link
Member

Choose a reason for hiding this comment

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

The pass is a no-op (no operation) and should be removed. I think it's from the template.

app/core/migrations/0030_user_user_status.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I realize there's only 1 migration in the original PR and it's definitely the right thing to do to add new commits in response to PR change requests.

Now that there's 3 migrations, you should add another commit to merge them into one. Here's a guide that's meant to help in this case. It's still in an issue right now. Maybe you can add some feedback/suggestion comment to it if you try it.

@shmonks shmonks mentioned this pull request Nov 1, 2024
Open
dmartin4820
dmartin4820 approved these changes Nov 4, 2024
View reviewed changes
Copy link
Member

@dmartin4820 dmartin4820 left a comment

Choose a reason for hiding this comment

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

No other changes from me. Tested buildrun and test scripts with no issues as well. Good work.

fyliu
fyliu approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@fyliu fyliu left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for adding the user status type model!

@fyliu fyliu force-pushed the create-table-user-status-type-35 branch from a60e248 to 5033fb7 Compare November 13, 2024 18:24
@fyliu fyliu force-pushed the create-table-user-status-type-35 branch from 5033fb7 to 8225c29 Compare November 13, 2024 20:50
@fyliu
Copy link
Member

fyliu commented Nov 13, 2024

I decided to squash the commits before merging because there's migrations that's replaced in later commits. Squashing them removes those intermediate files from history so that rolling forward from any past commit would work.

@fyliu fyliu merged commit e3441c8 into hackforla:main Nov 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmartin4820 dmartin4820 dmartin4820 approved these changes

@ethanstrominger ethanstrominger ethanstrominger approved these changes

@fyliu fyliu fyliu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
P: PD: Project Board
Status: ✅Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create Table: user_status_type
4 participants
@del9ra @fyliu @ethanstrominger @dmartin4820