story/VOGRE-14 #11

Girik1105 · 2024-10-17T21:37:24Z

Guidelines for Pull Requests

If you haven't yet read our code review guidelines, please do so, You can find them here.

Please confirm the following by adding an x for each item (turn [ ] into [x]).

I have removed all code style changes that are not necessary (e.g. changing blanks across the whole file that don’t need to be changed, adding empty lines in parts other than your own code)
I am not making any changes to files that don’t have any effect (e.g. imports added that don’t need to be added)
I do not have any sysout statements in my code or commented out code that isn’t needed anymore
I am not reformatting any files in the wrong format or without cause.
I am not changing file encoding or line endings to something else than UTF-8, LF
My pull request does not show an insane amount of files being changed although my ticket only requires a few files being changed
I have added Javadoc/documentation where appropriate
I have added test cases where appropriate
I have explained any part of my code/implementation decisions that is not be self-explanatory

Please provide a brief description of your ticket

Projects and texts

Description
The text import button should be for projects, not in the menu right now. Or if it stays in the main menu, the user should have to select a project to put the text in.
Actually, I think the way it currently is something is off. Texts should be part of projects, so the user goes to a project and sees all texts in that project. Not under “Texts”.

VOGRE-14

Projects should be sharable with other people.

Description
I want to be able to another user to my project and then they have access to all the texts and annotations in the project. And they should be able to add new texts and annotate them.

VOGRE-15

users should only see their own projects and projects that have been shared with them

VOGRE-24

Are there any other pull requests that this one depends on?

Anything else the reviewer needs to know?

Text URLS now contain the project instead of being passed as parameters.
Since there shouldn't be an instance where a text exists without a project, a user will be prompted to select a project when he is importing a text directly from a repository. A user can also directly add texts from a project.
Collaborators will be able to view existing texts and the annotations on them when added to a project as a collaborator, and if a collaborator is removed they will no longer has access to the text but their annotations won't be deleted

…oject import in view, need to remove redundancy and circular imports, make context consistent

…or directly from projects with add text

…ry texts

…aborator view and url, updated template

…ncorporate messages

jdamerow · 2024-10-30T15:56:38Z

annotations/views/project_views.py

@@ -36,6 +36,7 @@ def view_project(request, project_id):
    order_by = request.GET.get('order_by', 'title')
    texts = project.texts.all().order_by(order_by)\
                         .values('id', 'title', 'added', 'repository_source_id')
+    print(texts)


jdamerow · 2024-11-14T16:50:30Z

annotations/templates/annotations/project_list.html

-            href="{% url "view_project" project.id %}">
+            {% if next_url %}
+                <a class="list-group-item"
+                href="{{ next_url }}?project_id={{ project.id }}">


why does the project.id need to be passed here?

This will redirect the user to the text with ?project_id= which is later used by other views to retrieve the project id

jdamerow · 2024-11-14T16:51:01Z

annotations/views/repository_views.py

    repository = get_object_or_404(Repository, pk=repository_id)
+    user = None if isinstance(request.user, AnonymousUser) else request.user
+    project_id = request.GET.get('project_id')


merge with line 234

jdamerow · 2024-11-14T16:51:39Z

annotations/views/repository_views.py

+        # Filter texts within each collection that belong to the repository
+        collection_texts = collection.texts.filter(repository=repository).order_by('-added')
+        if collection_texts.exists():
+            texts_by_project[collection] = list(collection_texts)


does this mean that all texts in a collection will be loaded here? Those should be paginated, since they might be a lot.

…o see if text is already associated with project, merged project id in context in repository_details view

…ve projects

…rls, and templates

…ter to project

…ovements

jdamerow · 2024-11-21T16:38:41Z

annotations/exceptions.py

+
+    # Get custom error message from the PermissionDenied exception
+    # raise PermissionDenied("Custom message")
+    error_message = str(exception)


if None is passed as exception, this will make error_message "None", won't it?

jdamerow · 2024-11-21T16:43:10Z

annotations/utils.py

+        order_field = default_field
+        order_direction = ''
+        order_by = default_field


there are two fields here that seem to hold the same value (order_field, order_by), why are they both required?

jdamerow · 2024-11-21T16:44:08Z

annotations/utils.py

+
+    # Get order_by from request, default to default_field
+    order_by = request.GET.get('order_by', default_field)


you probably want to make 'order_by' also a parameter (with a default value), so it could be used for requests with a different parameter name.

jdamerow · 2024-11-21T17:00:31Z

annotations/views/project_views.py

    """
+    project = get_object_or_404(
+        _annotate_project_counts(TextCollection.objects),
+        pk=project_id


I'm so confused by this, doesn't get_object_or_404 expect a class, not a queryset? A

get_object_or_404 can also accept a queryset. The utility function _annotate_project_counts(TextCollection.objects) returns an annotated queryset, where I have added fields like num_texts, num_relations, and num_collaborators. By passing this annotated queryset to get_object_or_404 ensures that these additional fields are included in the retrieved project object (mainly used fore rendering the project stats)

https://docs.djangoproject.com/en/5.1/topics/http/shortcuts/#get-object-or-404

hm, I think there needs to be a comment here, especially since it would be more obvious why you do that it here are many projects and you want to annotate them all, but this is only requesting one single one. It also doesn't help that it's the "TextCollection" class but the id is "project_id". So there should be an explanatory comment why this is used over just getting the counts directly from the requested project (the way it's written makes it a lot harder to understand what's going on).

annotations/views/project_views.py

jdamerow · 2024-11-21T17:07:14Z

annotations/views/repository_views.py

+    user = None if isinstance(request.user, AnonymousUser) else request.user
+
+    # Fetch text collections (projects) owned by the current user
+    user_owned_collections = TextCollection.objects.filter(ownedBy=user)


is this used anywhere?

jdamerow · 2024-11-21T17:08:18Z

annotations/views/repository_views.py

+    if not project_id:
+        return redirect(f"{reverse('list_projects')}?redirect_to_text_import=True&repository_id={repository_id}&group_id={group_id}&text_key={text_key}")
+
+    print("project_id", project_id)


jdamerow · 2024-11-21T17:09:33Z

annotations/views/rest_views.py

+
+        # Allow GET requests for authenticated users
+        if request.method in ['GET', 'HEAD', 'OPTIONS']:
+            return True


I'm confused, why would it matter if this is a GET or POST request?

Hm, ok, I think I slowly understand, I think it's a naming issue. The class name suggests this will just check if the user is collaborator and owner, not that it checks specific texts for edit permissions (which I think this is doing?). Either it should check in general if the user can see/edit the text or the class name needs to be way more specific. In either case, the class name needs to be changed though.

jdamerow · 2024-11-21T17:15:40Z

annotations/views/rest_views.py

+        # Check if user is owner or collaborator of the text's collection
+        text = None
+        if hasattr(obj, 'occursIn'):
+            text = obj.occursIn


what is obj?

…r has obj permission

jdamerow · 2024-11-27T19:43:51Z

annotations/models.py

-        return reverse('annotate', args=[self.id])
+        """Get the absolute URL for viewing/annotating this text."""
+        # Get the first project this text belongs to
+        project = self.partOf.first()


this doesn't seem right. Wouldn't that mean that if a text is in multiple projects of multiple users, a user might see a text in a project that is not their own?

jdamerow · 2024-11-27T22:23:14Z

annotations/views/project_views.py

    """
+    project = get_object_or_404(
+        _annotate_project_counts(TextCollection.objects),
+        pk=project_id


hm, I think there needs to be a comment here, especially since it would be more obvious why you do that it here are many projects and you want to annotate them all, but this is only requesting one single one. It also doesn't help that it's the "TextCollection" class but the id is "project_id". So there should be an explanatory comment why this is used over just getting the counts directly from the requested project (the way it's written makes it a lot harder to understand what's going on).

…levant texts

diging-jenkins · 2024-12-05T17:36:38Z

Can one of the admins verify this patch?

Girik1105 added 11 commits October 15, 2024 12:04

[VOGRE-14] Added add to project button in annotate template, fixed pr…

88d77bd

…oject import in view, need to remove redundancy and circular imports, make context consistent

[VOGRE-14] Added workflow to add projects from texts while importing …

82d2245

…or directly from projects with add text

[VOGRE-14] Updated template to show texts of a group and not reposito…

5953dbe

…ry texts

[VOGRE-14] Change title based on how the page is being fetched

35ddefb

[VOGRE-14] Show only texts collections (projects) user has access to

bee50cf

[VOGRE-14] Improved UI

732dacf

[VOGRE-15] Added collaborators field in project model, added add coll…

09bd04a

…aborator view and url, updated template

[VOGRE-15] Fixed ordering in template and view

d3b83c8

[VOGRE-15] Added remove functionality, updated view and template to i…

71fd65e

…ncorporate messages

[VOGRE-14] Removed debug print statement

8312386

[VOGRE-15] Added default profile picture in projects

4520911

jdamerow requested changes Nov 14, 2024

View reviewed changes

jdamerow closed this Nov 14, 2024

Girik1105 added 3 commits November 14, 2024 10:26

[VOGRE-14] Removed project_id in next_url, added check in annotator t…

00958ec

…o see if text is already associated with project, merged project id in context in repository_details view

[VOGRE-14] Added pagination

46b6336

[VOGRE-14] Added font awesome cdn

9fc8341

Girik1105 reopened this Nov 14, 2024

Girik1105 added 9 commits November 14, 2024 10:58

[VOGRE-14] Merged develop and resolved conflicts

bc40356

[VOGRE-15] Improved UI

6e61d86

[VOGRE-24] Only project owner and collaborator can see their respecti…

df4eb24

…ve projects

[VOGRE-24] Added project stats

081425e

[VOGRE-24] Improved Stats rendering and UI

e594e7a

[VOGRE-24] project and text handling

5ba76a1

[VOGRE-14] Fixed next url issue

7bc0ea9

[VOGRE-24] Improved project list ui

249f2da

[VOGRE-24] Added links to user profiles in project sidebar

5863dc5

Girik1105 closed this Nov 15, 2024

Girik1105 added 3 commits November 15, 2024 13:54

[VOGRE-14] Reformatted project and text handling by updating views, u…

5a9560e

…rls, and templates

[VOGRE-14] fixed stats

2b10e54

[VOGRE-14][VOGRE-15] Merged story/VOGRE-15

0d46601

Girik1105 added 7 commits November 19, 2024 09:14

[VOGRE-14] Fixed text import from repository to project and collabora…

8a77e39

…ter to project

[VOGRE-14] added collaborator projects in your texts under repos

68c9f57

[VOGRE-14] Merged develop

fecc901

[VOGRE-14] Added custom template for permission denied errors

d23fe46

[VOGRE-14] Text not accessible if user is not owner or collaborator

1a14675

[VOGRE-14] Added collaboration projects in dashboard and made ui impr…

b99fbda

…ovements

[VOGRE-14] Fixed repository source id and some UI changes in Dashboard

e1d90cc

Girik1105 reopened this Nov 19, 2024

This was referenced Nov 20, 2024

story/VOGRE-7 #15

Closed

story/VOGRE-12 #14

Merged

jdamerow requested changes Nov 21, 2024

View reviewed changes

jdamerow closed this Nov 21, 2024

Girik1105 added 6 commits November 21, 2024 13:43

[VOGRE-14] Fixed ordering util and in function

5532eac

[VOGRE-14] Added defualt image froms settings

c3c6b4c

[VOGRE-14] removed print statement

7e56845

[VOGRE-14] removed extra query in repo view

8b3fe83

[VOGRE-14] Added pagination

d3e0412

[VOGRE-14] Renamed permission class, added comments, better naming fo…

280f4bb

…r has obj permission

Girik1105 reopened this Nov 21, 2024

jdamerow requested changes Nov 27, 2024

View reviewed changes

jdamerow closed this Nov 27, 2024

Girik1105 added 4 commits December 3, 2024 11:12

Merge branch 'develop' into story/VOGRE-14

5c770ab

[VOGRE-14] commented code explaining project id and textcollection

b7c532a

[VOGRE-14] Updating absolute url - Needs a better fix to only show re…

afac174

…levant texts

[VOGRE-14] Fixed absolute url and relation querying based on ownership

ffefdd4

Girik1105 reopened this Dec 5, 2024

[VOGRE-14] Fixed quering in dashboard

57c6316

jdamerow merged commit 9442b51 into develop Dec 12, 2024

jdamerow deleted the story/VOGRE-14 branch December 12, 2024 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

story/VOGRE-14 #11

story/VOGRE-14 #11

Girik1105 commented Oct 17, 2024 •

edited

Loading

jdamerow Oct 30, 2024

jdamerow Nov 14, 2024

Girik1105 Nov 14, 2024

jdamerow Nov 14, 2024

jdamerow Nov 14, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

Girik1105 Nov 21, 2024

jdamerow Nov 27, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

jdamerow Nov 21, 2024

jdamerow Nov 27, 2024

jdamerow Nov 27, 2024

diging-jenkins commented Dec 5, 2024


		# Get order_by from request, default to default_field
		order_by = request.GET.get('order_by', default_field)

story/VOGRE-14 #11

story/VOGRE-14 #11

Conversation

Girik1105 commented Oct 17, 2024 • edited Loading

Guidelines for Pull Requests

Please provide a brief description of your ticket

Are there any other pull requests that this one depends on?

Anything else the reviewer needs to know?

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diging-jenkins commented Dec 5, 2024

Girik1105 commented Oct 17, 2024 •

edited

Loading