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

Remove everything to do with topic_needs_review #698

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

Ngoakor12
Copy link
Contributor

@Ngoakor12 Ngoakor12 commented Feb 12, 2024

Related issues: [please specify]

Description:

What are you up to? Fill us in :)

Screenshots/videos

I solemnly swear that:

  • My code follows the style guidelines of this project
  • I have merged the develop branch into my branch and fixed any merge conflicts
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have tested new or existing tests and made sure that they pass

Copy link
Contributor

@kingraphaii kingraphaii left a comment

Choose a reason for hiding this comment

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

LGTM

I have a question though, there's still some refs in the "export_curriculum.py" and "import_curriculum.py" scripts. "topic_needs_review": o.topic_needs_review,, topic_needs_review, should we not remove those too

@Ngoakor12
Copy link
Contributor Author

LGTM

I have a question though, there's still some refs in the "export_curriculum.py" and "import_curriculum.py" scripts. "topic_needs_review": o.topic_needs_review,, topic_needs_review, should we not remove those too

My thinking was that we already had the objects fetched from the above lines and that accessing them while they're already fetched shouldn't make any new db calls. But I've changed it anyway, just in case.

Copy link
Collaborator

@sheenarbw sheenarbw left a comment

Choose a reason for hiding this comment

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

This is good, but it's incomplete.

Ther is a topic_needs review field on the ContentItem model. There are a whole lot of things that expect that column to exist. All that stuff must go.

If you are running vscode, open the backend directory and Ctrl+Shift+F to find all mentions of topic_needs_review. Those should all. The only mentions that can remain are those in the api serialisers. Everything else is technical debt

backend/curriculum_tracking/models.py Outdated Show resolved Hide resolved
backend/curriculum_tracking/models.py Show resolved Hide resolved
backend/curriculum_tracking/tests/test_api_views.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kingraphaii kingraphaii left a comment

Choose a reason for hiding this comment

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

A bunch of tests are failing because of the recent changes.

@Ngoakor12
Copy link
Contributor Author

A bunch of tests are failing because of the recent changes.

My bad, I'll have a look. I believe I just ran the frontend tests

@Ngoakor12
Copy link
Contributor Author

That should do it. Please reply with screenshots if the issue still persists so I know which tests are giving you issues

@kingraphaii
Copy link
Contributor

Here is one of them, basically ran python manage.py test to test everything:

..[Tilde-backend] [2024-02-28 06:27:11] INFO [logging_middleware:21] anonomous [GET] /api/activity_log_entries/
E
======================================================================
ERROR: test_get_list (activity_log.tests.test_api_views.TestLogEntryViewSet.test_get_list)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
psycopg2.errors.NotNullViolation: null value in column "topic_needs_review" violates not-null constraint
DETAIL:  Failing row contains (1, P, something awesome part 1, something-awesome-part-1, https://raw.githubusercontent.com/Umuzi-org/tech-department/mast..., null, R, https://github.com/Umuzi-org/bwahahhahaaaa, null, null, t, null, null, null, null, null).


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

Traceback (most recent call last):
  File "/Users/kingraphaii/Dev/Tilde/backend/test_mixins.py", line 48, in test_get_list
    instance = self.verbose_instance_factory()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/activity_log/tests/test_api_views.py", line 111, in verbose_instance_factory
    card = AgileCardFactory()
           ^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/base.py", line 40, in __call__
    return cls.create(**kwargs)
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/base.py", line 528, in create
    return cls._generate(enums.CREATE_STRATEGY, kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/django.py", line 121, in _generate
    return super()._generate(strategy, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/base.py", line 465, in _generate
    return step.build()
           ^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 270, in build
    step.resolve(pre)
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 211, in resolve
    self.attributes[field_name] = getattr(self.stub, field_name)
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 356, in __getattr__
    value = value.evaluate_pre(
            ^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/declarations.py", line 67, in evaluate_pre
    return self.evaluate(instance, step, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/declarations.py", line 457, in evaluate
    return step.recurse(subfactory, extra, force_sequence=force_sequence)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 228, in recurse
    return builder.build(parent_step=self, force_sequence=force_sequence)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 270, in build
    step.resolve(pre)
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 211, in resolve
    self.attributes[field_name] = getattr(self.stub, field_name)
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 356, in __getattr__
    value = value.evaluate_pre(
            ^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/declarations.py", line 67, in evaluate_pre
    return self.evaluate(instance, step, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/declarations.py", line 457, in evaluate
    return step.recurse(subfactory, extra, force_sequence=force_sequence)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 228, in recurse
    return builder.build(parent_step=self, force_sequence=force_sequence)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/builder.py", line 274, in build
    instance = self.factory_meta.instantiate(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/base.py", line 317, in instantiate
    return self.factory._create(model, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/factory/django.py", line 174, in _create
    return manager.create(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/query.py", line 447, in create
    obj.save(force_insert=True, using=self.db)
  File "/Users/kingraphaii/Dev/Tilde/backend/curriculum_tracking/models.py", line 318, in save
    super(ContentItem, self).save(*args, **kwargs)
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/base.py", line 753, in save
    self.save_base(using=using, force_insert=force_insert,
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/base.py", line 790, in save_base
    updated = self._save_table(
              ^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/base.py", line 895, in _save_table
    results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/base.py", line 933, in _do_insert
    return manager._insert(
           ^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/query.py", line 1254, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/models/sql/compiler.py", line 1397, in execute_sql
    cursor.execute(sql, params)
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 66, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 75, in _execute_with_wrappers
    return executor(sql, params, many, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 79, in _execute
    with self.db.wrap_database_errors:
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/Users/kingraphaii/Dev/Tilde/backend/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.IntegrityError: null value in column "topic_needs_review" violates not-null constraint
DETAIL:  Failing row contains (1, P, something awesome part 1, something-awesome-part-1, https://raw.githubusercontent.com/Umuzi-org/tech-department/mast..., null, R, https://github.com/Umuzi-org/bwahahhahaaaa, null, null, t, null, null, null, null, null).


----------------------------------------------------------------------

@Ngoakor12
Copy link
Contributor Author

I don't have any issues running all the tests now.

I believe those errors arise because the PR changes the model properties, so migrations must be made.

Running:

python manage.py makemigrations && python manage.py migrate

should resolve the issues.

@sheenarbw sheenarbw merged commit e7bee06 into develop Mar 1, 2024
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.

3 participants