Create table project sdg xref 66 #385

dmartin4820 · 2024-09-19T05:46:43Z

Fixes #66

What changes did you make?

Created project to sdg many-to-many relationship by updating both project and sdg models
Created many-to-many test as well as the other standard test procedures
Updated how-to with an example of a many-to-many test

Todo

When this PR is merged, release the dependency on issue docs: How to combine migrations before merging a PR #282

dmartin4820 · 2024-09-30T02:28:46Z

Still trying to resolve the merging of the current peopledepot main branch. I'm able to resolve any conflicts between files in my local repo and the incoming changes from the main branch, but I'm getting issues with data migrations.

I went through some of the steps here #378 (comment). When I run step 7 of the linked comment, the error output is shown below. I also ran step 5 and it output CommandError: core's max_migration.txt does not seem to contain a merge conflict. So, it didn't seem necessary in my case.

It appears to fail when applying core 0026. I'm going to try doing the migration without my changes. If that doesn't work, apparently I'm missing something and will require some time to figure out what's really happening. @fyliu @del9ra Any help is appreciated if you were able to resolve some of the data issues.

+ docker-compose exec -T web python manage.py makemigrations
No changes detected
+ docker-compose exec -T web python manage.py migrate core
Operations to perform:
  Apply all migrations: core
Running migrations:
  Applying core.0026_permissiontype_rank_alter_permissiontype_name_and_more...Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
psycopg2.errors.UniqueViolation: could not create unique index "core_permissiontype_rank_key"
DETAIL:  Key (rank)=(0) is duplicated.


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/src/app/manage.py", line 23, in <module>
    main()
  File "/usr/src/app/manage.py", line 19, in main
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 106, in wrapper
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/commands/migrate.py", line 356, in handle
    post_migrate_state = executor.migrate(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/executor.py", line 135, in migrate
    state = self._migrate_all_forwards(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/executor.py", line 167, in _migrate_all_forwards
    state = self.apply_migration(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/executor.py", line 252, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/migration.py", line 132, in apply
    operation.database_forwards(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/operations/fields.py", line 108, in database_forwards
    schema_editor.add_field(
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/base/schema.py", line 713, in add_field
    self.execute(sql, params)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/postgresql/schema.py", line 48, in execute
    return super().execute(sql, None)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/base/schema.py", line 201, in execute
    cursor.execute(sql, params)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 102, in execute
    return super().execute(sql, params)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 84, in _execute
    with self.db.wrap_database_errors:
  File "/usr/local/lib/python3.10/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/usr/local/lib/python3.10/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
django.db.utils.IntegrityError: could not create unique index "core_permissiontype_rank_key"
DETAIL:  Key (rank)=(0) is duplicated.

Just resolved. Fang suggested deleting my local database. We do this by running the command: ./scripts/buildrun.sh -v. The -v argument will remove Docker volumes, and the local database by consequence.

del9ra

Everything looks great!

The PR was done with the correct branch.
The issue was linked properly.
The code changes were applied correctly.
Thank you for your valuable contribution! The documentation for the many-to-many relationship is really helpful for me personally.

dmartin4820 · 2024-10-01T02:47:09Z

@fyliu Del took a look at the PR and didn't have issues. We wanted to check with you to see if you had any additional comments or improvements. Otherwise, I'll squash and merge the PR and move on to other issues.

dmartin4820 · 2024-10-07T00:09:38Z

The most recent force-push was an update to the documentation to reflect the unnecessary "through" field in the Project model for many-to-many relationships without additional fields on the "join table" between two models.

fyliu

Thanks for working though this new type of issue with related models! Great job documenting how to test it.

I think there's a problem with the api test where it always passes no matter what. Can you take a look?

fyliu · 2024-10-08T21:40:09Z

app/core/tests/test_api.py

+    proj_res = auth_client.get(PROJECT_URL)
+    sdg_res = auth_client.get(SDG_URL)
+
+    assert filter(lambda proj: str(proj["uuid"]) == str(project.pk), proj_res.data)


It looks like this is always going to pass since it's checking if the projects list contains the project model's pk.

I think you might have wanted to check if the projects list has a project that matches the sdg.projects models or one of them. I haven't given it much though on how to best test this.

This seems to work from my trial and error attempts:

assert proj_res.data[0]["sdgs"][0] == sdg.name

Possible missing API functionality

I was thinking "How would an API client assign an sdg to a project?" Do that API call and then check in the database to make sure it's really assigned. This might reveal that you need more code (in the serializer or somewhere) to help make that assignment.

Also, "How can an API client get the projects related to an sdg?" I found the StringRelatedField method as well, which returns the related project names. Maybe that's enough. It feels like we need something more than the name of the related sdg, like the pk instead, or as well. Suppose someone's doing an audit and wants all the projects that are working on a specific sdg (sustainability goal). Maybe they want the project urls in addition to the names.

What do you think of these? I'm pretty sure the first one is a must, even though we don't have the explicit requirement to have it.

The generic thing to do is to return the pks of all the related objects so that a client can go look them up if they want the details like project urls.

We should bring this up at the meeting and push for some API design. With Django, we get the single table CRUD actions for cheap, but related data requires code for both assignment and retrieval in each direction. We don't need to implement them if they're not going to end up being called by a client.

Thanks for catching this. Going forward, I'll test both cases, with and without the code changes to prevent this.

As for the 2 points you made, I agree that 1 is absolutely required and 2 has to be discussed more. It was something that came up while I was working on it, but it's not built into the docs or mandated in the task.

More specifically on point 2, I can imagine a case where the user may want to view an sdg and related projects on a single page. In that case you likely want more information than just the name, especially considering the project has many other fields. Although, I have no clue what the end user is doing as I haven't been to those meetings. I'm just implementing the models and relationships 😄.

It might take some research to see exactly how to implement more complex CRUD like this and document that, possibly a task itself. Then, creating tasks to actually implement it.

Going back over what I did originally with the filter function: I did filter(lambda proj: str(proj["uuid"]) == str(project.pk), proj_res.data) to make sure the test case doesn't assume I know how many projects are in the database at the time. The current change clearly still needs work, but it would make it more robust to changes if and when someone adds more projects in the pytest fixture. Let me know what you think.

I plan to implement the change suggested here:

assert proj_res.data[0]["sdgs"][0] == sdg.name

but this is after I find the correct sdg in the proj_res. Maybe I'm overdoing or overthinking it, but I'm willing to work together on something that works. I'll push new changes so you can see what I'm thinking as a whole.

dmartin4820 · 2024-10-14T01:42:00Z

Updated the test with suggested changes and more. The use of the filter function is removed in favor of the function here, for clarity. We should still come up with some agreement on what we want to test and how.

I also updated the docs to reflect the changes.

del9ra · 2024-10-21T18:09:27Z

docs/how-to/add-model-and-api-endpoints.md

+??? note "Updating models.py for many-to-many relationships"
+    For adding many-to-many relationships with additional fields, such as `ended_on`, we can add
+
+    ```python title="app/core/tests/test_models.py" linenums="1"


I believe you meant core/models.py instead of app/core/tests/test_models.py.

del9ra · 2024-10-21T18:10:10Z

docs/how-to/add-model-and-api-endpoints.md

+
+    For adding many-to-many relationships without additional fields, we can just add
+
+    ```python title="app/core/tests/test_models.py" linenums="1"


Same here core/models.py

fyliu

Thanks for fixing the api test!

I have just a few more nitpicks.

fyliu · 2024-10-23T10:39:03Z

app/core/tests/test_api.py

@@ -10,6 +10,7 @@
 pytestmark = pytest.mark.django_db

 USER_PERMISSIONS_URL = reverse("user-permission-list")
+PROJECT_URL = reverse("project-list")


It would be better to make the variable name plural like the other *-list ones here.

fyliu · 2024-10-23T10:40:47Z

app/core/tests/test_api.py

+
+
+def test_project_sdg_xref(auth_client, project, sdg, sdg1):
+    def get_object(data, target_uuid):


Suggestion: I would change the "data" name into something like "objs" to make it clear it's expecting to be passed a list of objects.

fyliu · 2024-10-23T10:43:26Z

app/core/tests/test_api.py

+    assert test_proj["sdgs"][0] == sdg.name
+    assert test_proj["sdgs"][1] == sdg1.name
+
+    sdg.projects.add(project)


Isn't sdg and project already related from above?

Yes, they should be related through the add statements from above. I removed that line now and confirmed that the GET request returns sdgs and the associated projects list.

FYI: made the update and it complained about objs as a name (something about clarifying it). My terminal output is gone now, so I don't know the exact error. Instead of objs, I renamed to objects and updated the changes I made in the docs as well.

fyliu · 2024-10-23T11:14:18Z

app/core/tests/test_api.py

+    test_proj = get_object(proj_res.data, project.uuid)
+    assert test_proj is not None
+    assert len(test_proj["sdgs"]) == 2
+    assert test_proj["sdgs"][1] != sdg.name


Maybe use the in keyword for these asserts?

dmartin4820 · 2024-10-29T04:25:23Z

Made the changes suggested. I also tried to make corresponding updates to the add-model-and-endpoints.md as well.

One minor thing: I updated SDG_URL to SDGS_URL here. I only bothered to change this one and the affected tests since this was directly related to this task. There may be other non-pluralized URL variables.

If we want to and it's necessary, this could be a good place to define how the CREATE, UPDATE, etc. request will implemented. Otherwise, more suggestions/comments are welcome on what is currently here.

fyliu · 2024-10-31T18:42:53Z

Made the changes suggested. I also tried to make corresponding updates to the add-model-and-endpoints.md as well.

Thanks! Will look at it in the review.

One minor thing: I updated SDG_URL to SDGS_URL here. I only bothered to change this one and the affected tests since this was directly related to this task. There may be other non-pluralized URL variables.

I just realized this myself while reading throught @del9ra's PR. Looks like many of these made it through reviews. Yes, just change the one relevant to this issue and we can make another issue to fix the others.

If we want to and it's necessary, this could be a good place to define how the CREATE, UPDATE, etc. request will implemented. Otherwise, more suggestions/comments are welcome on what is currently here.

Yes. I think we need to figure this part out.

del9ra

Fantastic job! Well done @dmartin4820 Thank you.

fyliu

Thank you @dmartin4820. Looks great!

shmonks · 2024-11-08T01:25:49Z

@fyliu it looks like you approved this - can you merge it or does something else need to be done?

fyliu · 2024-11-12T22:11:25Z

Just to explain what's happened in the above actions:

I combined the code changes into one commit and left the documentation commit alone. THen I rebased it onto hackforla/main.

But I found out that hackforla/main contains commits that don't pass linting, so fixed them "in-place" and force-pushed that. It's probably not necessary since they're not code changes and won't break anything. But they would be exceptions to the "all commits should lint" goal of the hackforla/main branch, so I did that.

Then I rebased the PR branch to hackforla/main again and force-pushed.

shmonks requested a review from del9ra September 20, 2024 00:06

fyliu assigned dmartin4820 Sep 20, 2024

fyliu mentioned this pull request Sep 26, 2024

Epic: collect needed documentation #378

Open

7 tasks

This comment was marked as resolved.

Sign in to view

dmartin4820 mentioned this pull request Sep 30, 2024

Create Table: project_sdg_xref #66

Closed

15 tasks

dmartin4820 force-pushed the create_table_project_sdg_xref_66 branch from e9d5461 to a3ff267 Compare September 30, 2024 02:43

del9ra approved these changes Sep 30, 2024

View reviewed changes

dmartin4820 requested a review from fyliu October 1, 2024 02:45

dmartin4820 mentioned this pull request Oct 4, 2024

People Depot Team Meeting Agenda 2024 #103

Open

dmartin4820 force-pushed the create_table_project_sdg_xref_66 branch from a3ff267 to 817d368 Compare October 7, 2024 00:07

fyliu requested changes Oct 8, 2024

View reviewed changes

dmartin4820 force-pushed the create_table_project_sdg_xref_66 branch from 817d368 to d24e691 Compare October 14, 2024 01:38

shmonks mentioned this pull request Oct 18, 2024

docs: How to combine migrations before merging a PR #282

Open

2 tasks

dmartin4820 requested review from fyliu and del9ra October 18, 2024 00:38

fyliu mentioned this pull request Oct 18, 2024

Create Table: project_program_area_xref #45

Open

16 tasks

del9ra reviewed Oct 21, 2024

View reviewed changes

del9ra requested changes Oct 21, 2024

View reviewed changes

fyliu requested changes Oct 23, 2024

View reviewed changes

shmonks mentioned this pull request Oct 24, 2024

Adding missing documentation on how to resolve migration conflicts #424

Open

14 tasks

dmartin4820 force-pushed the create_table_project_sdg_xref_66 branch from d24e691 to 09ddf2a Compare October 29, 2024 04:16

dmartin4820 requested review from del9ra and fyliu October 29, 2024 04:25

dmartin4820 mentioned this pull request Oct 31, 2024

Create Table: Accomplishment #48

Open

20 tasks

del9ra approved these changes Oct 31, 2024

View reviewed changes

fyliu approved these changes Oct 31, 2024

View reviewed changes

fyliu force-pushed the create_table_project_sdg_xref_66 branch from 09ddf2a to 0ba05bf Compare November 12, 2024 20:50

fyliu force-pushed the main branch from ee5a0a7 to 71b4ac9 Compare November 12, 2024 21:37

dmartin4820 added 2 commits November 12, 2024 13:45

feat: add many-to-many relationship to project: Sdg

bce99f3

docs(add-model): create tips for many-to-many relationships

9049e15

fyliu force-pushed the create_table_project_sdg_xref_66 branch from 0ba05bf to 9049e15 Compare November 12, 2024 21:50

fyliu merged commit 36007fd into hackforla:main Nov 12, 2024
1 check passed

vanessaavviles mentioned this pull request Nov 14, 2024

Missing Documentation on how to resolve migration conflicts in Github and Django #442

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create table project sdg xref 66 #385

Create table project sdg xref 66 #385

dmartin4820 commented Sep 19, 2024 •

edited

Loading

This comment was marked as resolved.

dmartin4820 commented Sep 30, 2024

del9ra left a comment

dmartin4820 commented Oct 1, 2024

dmartin4820 commented Oct 7, 2024

fyliu left a comment

fyliu Oct 8, 2024

dmartin4820 Oct 9, 2024

dmartin4820 Oct 14, 2024

dmartin4820 commented Oct 14, 2024 •

edited

Loading

del9ra Oct 21, 2024

del9ra Oct 21, 2024

fyliu left a comment

fyliu Oct 23, 2024

fyliu Oct 23, 2024

fyliu Oct 23, 2024

dmartin4820 Oct 29, 2024

dmartin4820 Oct 29, 2024

fyliu Oct 23, 2024

dmartin4820 commented Oct 29, 2024

fyliu commented Oct 31, 2024

del9ra left a comment

fyliu left a comment

shmonks commented Nov 8, 2024

fyliu commented Nov 12, 2024


		For adding many-to-many relationships without additional fields, we can just add

		```python title="app/core/tests/test_models.py" linenums="1"



		def test_project_sdg_xref(auth_client, project, sdg, sdg1):
		def get_object(data, target_uuid):

Create table project sdg xref 66 #385

Create table project sdg xref 66 #385

Conversation

dmartin4820 commented Sep 19, 2024 • edited Loading

What changes did you make?

Todo

This comment was marked as resolved.

dmartin4820 commented Sep 30, 2024

del9ra left a comment

Choose a reason for hiding this comment

dmartin4820 commented Oct 1, 2024

dmartin4820 commented Oct 7, 2024

fyliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Possible missing API functionality

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmartin4820 commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fyliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmartin4820 commented Oct 29, 2024

fyliu commented Oct 31, 2024

del9ra left a comment

Choose a reason for hiding this comment

fyliu left a comment

Choose a reason for hiding this comment

shmonks commented Nov 8, 2024

fyliu commented Nov 12, 2024

dmartin4820 commented Sep 19, 2024 •

edited

Loading

dmartin4820 commented Oct 14, 2024 •

edited

Loading