snapshot-setup #809

yashgoyal0110 · 2025-02-11T19:20:11Z

fixes: #800

arkid15r

I strongly suggest reconsidering the approach of working on tasks in general.

Please also add a minimal test coverage.

arkid15r · 2025-02-12T03:05:02Z

backend/apps/owasp/api/snapshot_service.py

I don't think we need this file.

arkid15r · 2025-02-12T03:06:09Z

backend/apps/owasp/management/commands/process_snapshots.py

+        """Get items created or updated within the snapshot timeframe."""
+        return model.objects.filter(
+            Q(created_at__gte=snapshot.start_at, created_at__lte=snapshot.end_at)
+            | Q(updated_at__gte=snapshot.start_at, updated_at__lte=snapshot.end_at)


We need new items only.

arkid15r · 2025-02-12T03:06:25Z

backend/apps/owasp/models/snapshot.py

The migration for this is missing.

arkid15r · 2025-02-12T03:06:43Z

backend/apps/owasp/models/snapshot.py

+    """Model representing a snapshot of data processing."""
+
+    class Status(models.TextChoices):
+        PENDING = "pending", _("Pending")


We don't do l10n yet.

arkid15r · 2025-02-12T03:16:25Z

backend/apps/owasp/models/snapshot.py

+    # Many-to-Many relationships
+    new_chapters = models.ManyToManyField("Chapter", related_name="snapshots", blank=True)
+    new_projects = models.ManyToManyField("Project", related_name="snapshots", blank=True)
+    new_issues = models.ManyToManyField("GitHubIssue", related_name="snapshots", blank=True)
+    new_releases = models.ManyToManyField("GitHubRelease", related_name="snapshots", blank=True)
+    new_users = models.ManyToManyField("GitHubUser", related_name="snapshots", blank=True)


This code won't work as GitHubIssue and other entities you referring to just don't exist. I expect a better quality of PRs for GSoC grade tasks. Please at least run your code locally.

yashgoyal0110 · 2025-02-16T21:03:46Z

Hey @arkid15r

Test files are added
Migrations are generated
file names are corrected
unwanted files are removed
code checked on local

arkid15r

This looks better but I haven't run this code yet. Please look into these when you get a chance:

arkid15r · 2025-02-17T04:33:52Z

backend/apps/owasp/models/snapshot.py

+    error_message = models.TextField(blank=True)
+
+    # Many-to-Many relationships
+    new_chapters = models.ManyToManyField("Chapter", related_name="snapshots", blank=True)


Let's make it owasp.Chapter for consistency

changes made 👍

arkid15r · 2025-02-17T04:34:21Z

backend/tests/owasp/models/snapshot_test.py

+        ),
+    ],
+)
+def test_str_representation(start_at, end_at, status, expected_str):


Could you use a class here too?

made the changes 👍

arkid15r · 2025-02-20T02:17:34Z

backend/apps/owasp/admin.py

+class SnapshotAdmin(admin.ModelAdmin):
+    list_display = ("start_at", "end_at", "status", "created_at", "updated_at")
+    list_filter = ("status", "start_at", "end_at")
+    search_fields = ("status", "error_message")
+    ordering = ("-start_at",)
+
+


Could you keep the formatting/style consistent with the current one? I mean the new class position within other classes and tuple formatting.

arkid15r · 2025-02-20T02:18:08Z

backend/apps/owasp/management/commands/process_snapshots.py

+            error_msg = f"Failed to process snapshot: {e}"
+            raise SnapshotProcessingError(error_msg) from e
+
+    def get_new_items(self, model, snapshot):


You need start/end dates only instead of the entire snapshot.

arkid15r · 2025-02-20T02:21:23Z

backend/apps/owasp/models/snapshot.py

+
+    start_at = models.DateTimeField()
+    end_at = models.DateTimeField()
+    status = models.CharField(max_length=20, choices=Status.choices, default=Status.PENDING)


Isn't 20 too much?

arkid15r · 2025-02-20T02:23:30Z

backend/apps/owasp/models/snapshot.py

+    status = models.CharField(max_length=20, choices=Status.choices, default=Status.PENDING)
+    created_at = models.DateTimeField(auto_now_add=True)
+    updated_at = models.DateTimeField(auto_now=True)
+    error_message = models.TextField(blank=True)


also add default=""

arkid15r · 2025-02-20T02:24:53Z

backend/apps/owasp/models/snapshot.py

+        ordering = ["-start_at"]
+        indexes = [
+            models.Index(fields=["status"]),
+            models.Index(fields=["start_at", "end_at"]),
+        ]


Remove these and add db_table and verbose_name_plural that we use for in other models.

arkid15r · 2025-02-20T02:26:58Z

backend/tests/owasp/models/snapshot_test.py

+        snapshot.__str__.return_value = expected_str
+        assert str(snapshot) == expected_str


This test doesn't make sense as you mock the __str__ and compare it with the value you return. You need to actually test the code of Snapshot model

arkid15r · 2025-02-20T02:28:20Z

backend/tests/owasp/models/snapshot_test.py

+        snapshot.status = "pending"
+        assert snapshot.status == "pending"


Same here, please make sure to test actual application code.

yashgoyal0110 requested a review from arkid15r as a code owner February 11, 2025 19:20

github-actions bot added the backend label Feb 11, 2025

yashgoyal0110 force-pushed the fix/snapshot-setup branch 2 times, most recently from 8232789 to 4b687e5 Compare February 11, 2025 19:35

snapshot-setup

54f5ebd

yashgoyal0110 force-pushed the fix/snapshot-setup branch from 4b687e5 to 54f5ebd Compare February 11, 2025 19:49

yashgoyal0110 added 2 commits February 12, 2025 01:44

snapshot-setup

2f65efd

snapshot-setup

58dd96b

arkid15r requested changes Feb 12, 2025

View reviewed changes

yashgoyal0110 and others added 3 commits February 12, 2025 10:30

Merge branch 'main' into fix/snapshot-setup

561a67d

Merge branch 'main' into fix/snapshot-setup

20e4322

commit

fe711d9

github-actions bot added the backend-tests label Feb 15, 2025

yashgoyal0110 and others added 5 commits February 16, 2025 20:52

Merge branch 'main' into fix/snapshot-setup

2550727

snapshot

bb94b5b

snapshot-setup

a4e132b

snapshot-setup

5a56bd9

snapshot

3d0bf28

yashgoyal0110 force-pushed the fix/snapshot-setup branch from 0037a2f to 3d0bf28 Compare February 16, 2025 19:47

snapshot

e358468

yashgoyal0110 force-pushed the fix/snapshot-setup branch from 18e85fa to e358468 Compare February 16, 2025 19:49

snapshot-changes

17fa869

yashgoyal0110 requested a review from arkid15r February 16, 2025 20:01

yashgoyal0110 added 2 commits February 17, 2025 02:27

snapshot tests

8d10f3b

snapshot tests

8bbf169

yashgoyal0110 force-pushed the fix/snapshot-setup branch from 2497634 to 8bbf169 Compare February 16, 2025 21:01

arkid15r requested changes Feb 17, 2025

View reviewed changes

yashgoyal0110 and others added 2 commits February 17, 2025 10:33

Merge branch 'main' into fix/snapshot-setup

5e071a4

snapshot changes

6b3a8c6

yashgoyal0110 requested a review from arkid15r February 17, 2025 05:19

Merge branch 'main' into fix/snapshot-setup

7c18be7

arkid15r requested changes Feb 20, 2025

View reviewed changes

yashgoyal0110 added 6 commits February 20, 2025 10:54

Merge branch 'main' into fix/snapshot-setup

b3f5e12

Merge branch 'main' into fix/snapshot-setup

b1b090b

commit

2837f4a

fixed

9425f7d

Merge branch 'main' into fix/snapshot-setup

bf154eb

tests

e7fde49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snapshot-setup #809

snapshot-setup #809

yashgoyal0110 commented Feb 11, 2025

arkid15r left a comment

arkid15r Feb 12, 2025

arkid15r Feb 12, 2025

arkid15r Feb 12, 2025

arkid15r Feb 12, 2025

arkid15r Feb 12, 2025

yashgoyal0110 commented Feb 16, 2025

arkid15r left a comment

arkid15r Feb 17, 2025

yashgoyal0110 Feb 17, 2025

arkid15r Feb 17, 2025

yashgoyal0110 Feb 17, 2025 •

edited

Loading

arkid15r Feb 20, 2025

arkid15r Feb 20, 2025

arkid15r Feb 20, 2025

arkid15r Feb 20, 2025

arkid15r Feb 20, 2025

arkid15r Feb 20, 2025

arkid15r Feb 20, 2025

		snapshot.__str__.return_value = expected_str
		assert str(snapshot) == expected_str

		snapshot.status = "pending"
		assert snapshot.status == "pending"

snapshot-setup #809

Are you sure you want to change the base?

snapshot-setup #809

Conversation

yashgoyal0110 commented Feb 11, 2025

arkid15r 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

yashgoyal0110 commented Feb 16, 2025

arkid15r 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

yashgoyal0110 Feb 17, 2025 • edited Loading

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

Choose a reason for hiding this comment

yashgoyal0110 Feb 17, 2025 •

edited

Loading