adding the notes table and versioning #171

femalves · 2023-08-16T21:23:19Z

I would really like your input this. I wrote the model but I'm not sure if it's the best approach. Please, let me know your thoughts.
I also made some changes to manage.py but I haven't fully reviewed those yet, but feel free to give me some pointers.

…usability"

;

kelockhart

Looking good so far! There are a few minor comments, but overall it's looking pretty solid, and the refactored code is much easier to read. I know I started this review before your latest commits, so you might have addressed some of this already - if so, disregard the comments that don't apply.

kelockhart · 2023-10-11T14:56:01Z

biblib/models.py

+    __versioned__ = {}
+    id = Column(Integer, primary_key=True)
+    content = Column(UnicodeText)
+    bibcode = Column(String(50), nullable=False)


I'd make this length 19, instead of 50

kelockhart · 2023-10-12T18:34:13Z

biblib/models.py

+
+        note = Notes(content=content, bibcode=bibcode, library_id=library.id)
+        session.add(note)
+        session.commit()


Generally we leave the session handling (session.add() and session.commit()) for the calling function. It can be done here, as you have it, but for consistency, let's move these to the calling function. To see what I mean, check how add_bibcodes on the Library class is being used.

kelockhart · 2023-10-12T19:06:48Z

biblib/manage.py

-        """
-        if not n_revisions: n_revisions = current_app.config.get('NUMBER_REVISIONS', 7)
+        if not n_revisions:
+            n_revisions = current_app.config.get('NUMBER_REVISIONS', 7)


Totally fine to treat notes the same as libraries for removing revisions, as you have here, for consistency and to keep the revisions tables from getting out of control. However, it's worth keeping an eye on the notes versions table and seeing how frequently notes are being changed and whether storing 7 revisions is enough - my intuition is that notes may have more frequent changes than libraries, and thus a user might need access to earlier versions. No need to change this now, but just something to keep in mind, and keep an eye on.

Okay. I'll keep that in mind.

kelockhart · 2023-10-12T20:20:20Z

biblib/views/library_view.py

+            return library_error
+
+        # If set to True, return notes in library
+        notes = request.args.get('notes', type=bool, default=True)


I realized we never added the allowed params to the docstring when we first added support for pagination (e.g. start, rows, etc. - the things returned via your self.load_parameters call. Those, plus this new notes param should be added to the docstring here. Eventually, you'll also need to update the API docs (https://ui.adsabs.harvard.edu/help/api/api-docs.html) to add this param, but we can talk about that later.

kelockhart · 2023-10-12T20:40:23Z

biblib/views/library_view.py

+
+    def process_solr(self, library, start, rows, sort, fl):
+        """
+        Processes the request for raw library 


Looks like a copy/paste error - just update the text

kelockhart · 2023-10-12T20:50:21Z

biblib/views/library_view.py

+            )
+        # Extract the canonical bibcodes and create a hashmap 
+        # in which the alternate bibcode is the key and the canonical bibcode is the value
+        alternate_bibcodes = cls.get_alternate_bibcodes(solr_docs)

        with current_app.session_scope() as session:


There was a session getting passed around in one of the calling functions - you should be able to reuse it here.

kelockhart · 2023-10-12T21:08:33Z

biblib/views/library_view.py

+
+    def is_library_public_or_has_special_token(self, library, request):
+        """
+        HTTP GET request that returns all the documents inside a given


Copy/paste error - update text

kelockhart · 2023-10-12T21:10:27Z

biblib/views/library_view.py

-            )
-            return err(NO_PERMISSION_ERROR)
+
+        current_app.logger.warning('Library: {0} is private'.format(library_id))


I know this is pre-existing code, but it's weird that it's generating a warning-level log for this - can you downgrade it to info-level?

kelockhart · 2023-10-12T21:21:29Z

biblib/tests/unit_tests/test_models.py

 from biblib.tests.base import TestCaseDatabase
+from sqlalchemy import exc
+import pytest


Is this leftover from testing? I don't see it in here

kelockhart · 2023-10-12T21:24:49Z

biblib/tests/unit_tests/test_views.py

@@ -1,16 +1,16 @@
 """
 Tests Views of the application
 """
-
+import json


Leftover from testing?

tjacovich

I think the merge with timestamp_sort looks good. Just had a couple of minor comments.

tjacovich · 2023-11-07T18:24:28Z

biblib/views/library_view.py

+        sort = request.args.get('sort', 'date desc')
+        #timestamp sorting is handled in biblib so we need to change the sort to something SOLR understands.
+        if sort in ['time asc', 'time desc']:
+            current_app.logger.debug("sort order is set to{}".format(sort))


I'd add a space in the log statement, but This all looks reasonable.

tjacovich · 2023-11-07T18:30:03Z

alembic/versions/08c9a177f639_notes_table.py

+    op.create_index(op.f('ix_notes_version_end_transaction_id'), 'notes_version', ['end_transaction_id'], unique=False)
+    op.create_index(op.f('ix_notes_version_operation_type'), 'notes_version', ['operation_type'], unique=False)
+    op.create_index(op.f('ix_notes_version_transaction_id'), 'notes_version', ['transaction_id'], unique=False)
+    op.create_table('notes',


This is just a readability thing, but I would put a space between the last create_index and the create_table to differentiate between calls for notes_version vs. notes

kelockhart

Sorry this took me so long! I have some high level questions and suggestions, along with a couple more detailed comments. Let's chat about this during our next meeting - a synchronous review will go faster on my part.

kelockhart · 2023-11-17T20:30:15Z

biblib/models.py

-        session.add(note)
-        session.commit()
-        return note
+        try: 


What is this try/except handling? Looks like you've got pretty good exception handling in this method already.

kelockhart · 2023-12-02T00:31:10Z

biblib/views/library_view.py

@@ -508,132 +397,92 @@ def process_library_request(self, data):
        library_notes:        <dict>    Dictionary of library notes, including orphan 
                                        notes (those not associated with a bibcode in the library)
        """
+        with current_app.session_scope() as session:
+            try:


In general, it's better to have as little code as possible inside a try - if something fails and there's a ton of code in there, it can be really hard to debug. Try to rewrite this to include only the bare minimum of what might fail

kelockhart · 2023-12-02T00:34:33Z

biblib/views/notes_view.py

+          - read
+        """
+
+        try: 


Same comment about the try here - I like how you're handling the separate types of exceptions separately, but better to wrap each line in the try/except separately

kelockhart · 2023-12-02T00:36:24Z

biblib/views/notes_view.py

+
+            current_app.logger.info('Getting note for document {0} in library {1}.'.format(document_id, library_id))
+
+            return response, 200 


Can you stick a line break between this line and the next? Just for readability - took me a sec to find this return statement

kelockhart · 2023-12-02T00:38:11Z

biblib/views/notes_view.py

+        The following type of user can update the content:
+
+          - owner
+          - admin


write type permissions should also be able to edit

kelockhart · 2023-12-02T00:39:27Z

biblib/views/notes_view.py

+        service_uid = self.helper_absolute_uid_to_service_uid(absolute_uid=user)
+
+        # Check the user's permissions
+        if not self.update_access(service_uid=service_uid,


update to write_access

kelockhart · 2023-12-02T00:41:25Z

biblib/views/notes_view.py

+        Permissions:
+        -----------
+        The following type of user can delete the note:
+          - owner


Let's let the admin and write permission types also delete the notes - both of these are able to delete records from a library, so it's more consistent that we also allow them to delete notes

kelockhart · 2023-12-02T00:41:40Z

biblib/views/notes_view.py

+                                'requesting to delete note for document {1} and library {2}.'
+                                .format(service_uid, document_id, library))
+
+        if not self.delete_access(service_uid=service_uid,


update to write_access

kelockhart · 2023-12-02T00:54:45Z

biblib/views/user_view.py

+            if shared_with_me: 
+                response['shared_with_me'] = shared_with_me
+
+        return response


We'll have to be careful with the deployment, when we're ready to do it - changing the keys in the response (libraries -> my_libraries/shared_with_me) will make this not backwards compatible, so both UIs will need to be updated. Alternatively, we could figure out a way to make this backwards compatible. Let's talk more about this.

kelockhart · 2023-12-02T01:02:43Z

biblib/views/user_view.py

-                output_libraries.append(payload)
-
-            return output_libraries
+                if (permissions and main_permission in ['owner']) or not permissions: 


Where is permissions set? I see permission.permissions but not just permissions, but maybe I'm missing it. Either way, I have some concerns about the or not permissions part being lumped in with the 'owner' permission. Depending on how permissions is assigned, either stick the or not permissions in the elif, or an else

kelockhart

Approved after fixing the extra log message, per our discussion

tjacovich

Looks good to me. Excited to see it in action.

adding the notes table and versioning

da909be

femalves requested a review from tjacovich August 16, 2023 21:23

femalves added 7 commits August 23, 2023 17:00

added the update notes functionality

ae1045f

added comments;

7549f7c

refactored get_documents_from_library;

e236f64

refactored the get method in library_view for better readability and …

8cb063c

…usability"

fixed all errors

cb580a6

get method

4bae434

removing wrong doc

788bd23

femalves requested a review from kelockhart September 27, 2023 20:04

femalves added 4 commits September 29, 2023 17:33

removing old migration

26111f3

adding new migrations

5adeef8

changing repr in model

0650ce0

adding tests and making reorganizing

ad9b842

femalves marked this pull request as ready for review October 11, 2023 21:04

femalves added 2 commits October 12, 2023 17:03

making code in the get method on LibraryView more consistent"

1396f2b

;

moving session creation to appropriate method

e8e02a5

kelockhart requested changes Oct 12, 2023

View reviewed changes

femalves added 4 commits October 16, 2023 18:11

creating new notes table and correcting bugs

f2007cd

adding new version and correcting docstrings

98af7c1

adding endpoints for notes

7f15dcb

adding library_count, my_libraries and shared_with_me to userview

c4d17d4

femalves requested a review from kelockhart October 23, 2023 18:28

femalves and others added 5 commits October 23, 2023 20:22

adding other sorting order

5a65626

changing get_libraries

09ebb2f

Merge branch 'master' into adding_notes

141675a

fixing conflicts

a419caa

adding tests

41f238f

tjacovich requested changes Nov 8, 2023

View reviewed changes

fixing minor things

d942fb7

kelockhart requested changes Dec 2, 2023

View reviewed changes

changing write accesses and cleaning up

d1b2677

kelockhart approved these changes Dec 8, 2023

View reviewed changes

removing log comment

58fcfee

femalves requested a review from tjacovich December 11, 2023 21:16

tjacovich approved these changes Dec 13, 2023

View reviewed changes

femalves merged commit f584fb5 into master Dec 14, 2023
1 check passed

femalves deleted the adding_notes branch December 14, 2023 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding the notes table and versioning #171

adding the notes table and versioning #171

femalves commented Aug 16, 2023

kelockhart left a comment

kelockhart Oct 11, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

femalves Oct 13, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

kelockhart Oct 12, 2023

tjacovich left a comment

tjacovich Nov 7, 2023

tjacovich Nov 7, 2023

kelockhart left a comment

kelockhart Nov 17, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart Dec 2, 2023

kelockhart left a comment

tjacovich left a comment

+                        - read
+                      """
+                      try:


		current_app.logger.info('Getting note for document {0} in library {1}.'.format(document_id, library_id))

		return response, 200

adding the notes table and versioning #171

adding the notes table and versioning #171

Conversation

femalves commented Aug 16, 2023

kelockhart 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

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

tjacovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelockhart 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

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

kelockhart left a comment

Choose a reason for hiding this comment

tjacovich left a comment

Choose a reason for hiding this comment