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

Create table project_program_area_xref #376

Conversation

del9ra
Copy link
Member

@del9ra del9ra commented Sep 5, 2024

Fixes #45

What changes did you make?

  • Created a model in Django
  • Wrote a test for the relationships this model will have with other models
  • Registered the model in admin.py
  • Wrote an API end point
  • Wrote API unit tests
  • Documented the endpoint

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

  • to create the project_program_area_xref table
  • to update a shared data store across hackforla.org, vrms, civictechjobs, and tables (onboarding) project

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

No changes

@del9ra del9ra changed the title 45 create table project program area xref Create table project_program_area_xref Sep 5, 2024
@dmartin4820 dmartin4820 self-requested a review September 6, 2024 00:41
@@ -20,11 +20,11 @@ RUN \
apt-get update \
&& apt-get install --no-install-recommends -yqq \
netcat=1.10-46 \
gcc=4:10.2.1-1 \
graphviz=2.42.2-5
Copy link
Member

Choose a reason for hiding this comment

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

To fix the issue we were encountering when running ./scripts/buildrun.sh, removing this line was just a local quick fix. Instead it may be better to specify graphviz=2.42.* so that the newest patch is installed instead (i.e. graphviz_2.42.2-5+deb11u1 instead of graphviz_2.42.2-5).

class ProjectProgramAreaXref(AbstractBaseModel):
project_id = models.ForeignKey(Project, on_delete=models.CASCADE)
program_area_id = models.ForeignKey(ProgramArea, on_delete=models.CASCADE)
created_date = models.DateTimeField("Created at", null=True, blank=True)
Copy link
Member

Choose a reason for hiding this comment

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

The inherited AbstractBaseModel class already creates a timestamp for creation; see here

Copy link
Member Author

Choose a reason for hiding this comment

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

Why did they use the DateTimeField in the Add the Model section of this guide: https://hackforla.github.io/peopledepot/how-to/add-model-and-api-endpoints/, and in the Project model as "completed_at", in the Event model as "start_time", and in the Affiliation model as "ended_at"?

Copy link
Member

Choose a reason for hiding this comment

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

@del9ra there's some translation required between the DB notation in the ERD and spreadsheet and the Django data fields. We use an abstract base class (ABC) so that all the models will have create and modify timestamps. In the database design, many but not all models have the timestamps. We decided to make that a standard thing at one point.

So you should ignore anything from the database table that does the same as create and update timestamps.

At the time, I was trying to decide what to call these standard timestamps, and I found some page which discussed "on" vs "at", where "on" is usually a date and "at" is usually a time. So I added "_at" to the end of the names. But eventually I found a similar TimeStampedModel ABC in the widely-used django package django-extensions. It would've been a good idea to use that too.

Copy link
Member

@fyliu fyliu Sep 8, 2024

Choose a reason for hiding this comment

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

Hmm... looks like this xref table doesn't have any extra data fields other than the relations (foreign keys). It means that we don't really need to create a Django model for this table at all. I'll have to go though the other issues and pick out similar issues before someone works on them. Like the issue where I noted this.

Copy link
Member

@fyliu fyliu Sep 8, 2024

Choose a reason for hiding this comment

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

@del9ra I'm sorry, but I need to change this issue from "create xref table" to "create many to many relation between Project and ProgramArea models". We don't have any existing code in the repository to serve as an example, but I can find some other examples if you want to work on it. This guide looks good for the Django model.

These may be helpful also:

Copy link
Member

Choose a reason for hiding this comment

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

Why did they use the DateTimeField in the Add the Model section of this guide: https://hackforla.github.io/peopledepot/how-to/add-model-and-api-endpoints/, and in the Project model as "completed_at", in the Event model as "start_time", and in the Affiliation model as "ended_at"?

@del9ra Did you get your question answered? I'm not sure why those were included either. This should be added to the meeting agenda if we can't resolve this. I'm not sure of how the decisions were made for completed, start, ended and so on. Looking at the ERD also shows that completed_at has a type of "date" versus "DateTimeField". It looks like the ERD also needs to be updated for at least Project, if not all the other tables.

Copy link
Member Author

@del9ra del9ra Sep 10, 2024

Choose a reason for hiding this comment

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

@del9ra I'm sorry, but I need to change this issue from "create xref table" to "create many to many relation between Project and ProgramArea models". We don't have any existing code in the repository to serve as an example, but I can find some other examples if you want to work on it. This guide looks good for the Django model.

These may be helpful also:

Sounds good to me. I’ll work on the many-to-many relation. Thanks for letting me know!

Copy link
Member Author

@del9ra del9ra Sep 30, 2024

Choose a reason for hiding this comment

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

@del9ra there's some translation required between the DB notation in the ERD and spreadsheet and the Django data fields. We use an abstract base class (ABC) so that all the models will have create and modify timestamps. In the database design, many but not all models have the timestamps. We decided to make that a standard thing at one point.

So you should ignore anything from the database table that does the same as create and update timestamps.

At the time, I was trying to decide what to call these standard timestamps, and I found some page which discussed "on" vs "at", where "on" is usually a date and "at" is usually a time. So I added "_at" to the end of the names. But eventually I found a similar TimeStampedModel ABC in the widely-used django package django-extensions. It would've been a good idea to use that too.

@fyliu I noticed that we have the ended_at field in the Affiliation model; ended in Permission model, project_sponsor_partner_xref, permission_history; ended_on in project_language_xref and project_sdg_xref. How does this work in this case? Do we need to create a single field name for all of them in the ABC model?

Copy link
Member

@fyliu fyliu Sep 30, 2024

Choose a reason for hiding this comment

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

@del9ra Thank you for bringing this up!
We could. It looks like the ended field applies to xref tables where the relationships ended but we're keeping the row around (soft delete).
On the one hand, we don't have a lot of those tables to apply it to, so we shouldn't do this for all the models. Then again, you pointed out that the few we have are already inconsistent in naming, so maybe we should do something to make/keep them consistent.

I'm thinking we should bring up the inconsistent naming to @ExperimentsInHonesty and @Neecolaa and at least make an issue (ER maybe) to fix it on some tables. Maybe make them all ended_at datetime stamps to be simpler and consistent with the other timestamps. It's more clear what day it is even with timezone differences.

We should bring this to the meeting and decide on whether or not to make an ABC model or something else to help force this consistency.

Here's a list of stakeholders and what they might care about most:

  • (developer) what's easier for developers to code with less required training and less error?
  • (reviewer) what's easier for reviewers to review with less required training and less error?
  • (project) what's a quicker fix?
  • (project) what's a better long term solution?

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.

Minor changes to version in Dockerfile and duplicate created timestamp field in model.

Also, the past pull requests I've looked at will merge into hackforla:main, unless @fyliu or someone else said different. My last create table PR was directly into hackforla:main.

@fyliu fyliu mentioned this pull request Sep 7, 2024
6 tasks
@del9ra
Copy link
Member Author

del9ra commented Sep 8, 2024

Minor changes to version in Dockerfile and duplicate created timestamp field in model.

Also, the past pull requests I've looked at will merge into hackforla:main, unless @fyliu or someone else said different. My last create table PR was directly into hackforla:main.

@fyliu Could you clarify this please?

@fyliu
Copy link
Member

fyliu commented Sep 8, 2024

@del9ra He means the to branch should be hackforla:main. Your from is good, but your to is set to another branch in hackforla. It should be from your feature branch in your fork to hackforla:main.

Here are screenshots from this and Denzal's PR

2024-09-08 01 52 57 github com 0668c51bf5e9
2024-09-08 01 54 44 github com bab2b93fdd5c

@dmartin4820
Copy link
Member

@del9ra He means the to branch should be hackforla:main. Your from is good, but your to is set to another branch in hackforla. It should be from your feature branch in your fork to hackforla:main.

Here are screenshots from this and Denzal's PR

2024-09-08 01 52 57 github com 0668c51bf5e9 2024-09-08 01 54 44 github com bab2b93fdd5c

If we want this to merge into main, I don't know the best/easiest way to do that since this PR has already been created. It may be possible to let it merge into [hackforla:45-create-table-project_program_area_xref, then merge that branch into main. The other way is to just create a new PR. I'm not sure there is a clean way to change the PR branch destination at this point, but the changes will still be able to go to the correct place regardless.

@shmonks shmonks assigned fyliu and unassigned del9ra and fyliu Sep 20, 2024
@shmonks
Copy link
Member

shmonks commented Sep 20, 2024

Closing this PR because it turns out that most xref tables can be created automatically by Django

@shmonks shmonks closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅Done
Development

Successfully merging this pull request may close these issues.

4 participants