Story/VOGRE-32 #33

rajvi-patel-22 · 2024-12-16T22:25:09Z

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

Workflow

User goes to concept list page

User selects a concept that has not be resolved

If there is a viaf url, Vogon will search Conceptpower for that viaf url and show the result. If there is exactly one result, user should be able to say “approve”, if there are more, user should select one

User should also have the chance to do a conceptpower search for a search term and select one of the results.

If there is no viaf url, user should get a search field to search conceptpower and then be able to select one of the results or do a new search

For all of these options, there should also be a button to create a new concept in conceptpower.

Once a concept has been selected and approved, Vogon concept is RESOLVED

We will likely only have “Pending” and “resolved” as concept states.

... Put ticket description here and add link to ticket ...
https://diging.atlassian.net/browse/VOGRE-32

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

Anything else the reviewer needs to know?

Below bug is fixed along with this PR:

Fix this.label for new concept name/label

diging-jenkins · 2024-12-16T22:25:12Z

Can one of the admins verify this patch?

jdamerow · 2024-12-19T14:32:23Z

concepts/lifecycle.py

+            response = requests.post(url=url, data=json.dumps(concept_data), auth=auth)
+
+            if response.status_code != requests.codes.ok:
+                raise RuntimeError(response.status_code, response.text)


this should happen in a conceptpower class not directly here.

jdamerow · 2024-12-19T14:32:30Z

concepts/lifecycle.py

@@ -230,10 +172,16 @@ def merge_with(self, uri):
        children_queryset.update(merged_with=target)

    def add(self):
+


remove empty line

jdamerow · 2024-12-19T14:34:18Z

concepts/lifecycle.py

+                if 'conceptEntries' in data:
+                    for concept_entry in data['conceptEntries']:
+                        concept = self.parse_concept(concept_entry)
+                        concepts.append(concept)


same as above, should not happen in this class

jdamerow · 2024-12-19T14:34:41Z

concepts/lifecycle.py

+                if 'conceptEntries' in data:
+                    for concept_entry in data['conceptEntries']:
+                        concept = self.parse_concept(concept_entry)
+                        concepts.append(concept)


jdamerow · 2024-12-19T14:35:04Z

concepts/lifecycle.py

+                    data = response.json()
+                    concept_entry = data.get('conceptEntries', [{}])[0]
+            else:
+                raise ValueError(f"Error fetching concept data: {response.status_code}")


same, there should be a conceptpower class that does all the communication with conceptpower (isn't there one already?)

jdamerow · 2025-01-08T14:37:26Z

concepts/conceptpower.py

+        # Give first priority to the class definition, if endpoint or namespace are defined (and not None).
+        if self.endpoint is None:
+            self.endpoint = kwargs.get(
+                "endpoint", "http://chps.asu.edu/conceptpower/rest/")


these should not be hard-coded; if a default is needed it should come from environment variables/settings.

jdamerow · 2025-01-08T14:37:31Z

concepts/conceptpower.py

+
+        if self.namespace is None:
+            self.namespace = kwargs.get(
+               "namespace", "{http://www.digitalhps.org/}")


jdamerow · 2025-01-08T14:38:12Z

concepts/conceptpower.py

+
+        response = requests.get(url, headers=headers, params=params)
+        concepts = []
+        if response.status_code == 200:


if not, we definitely want to log what else is returned and an error message if there is one.

also, you want to use a constant (requests probably has one) and not a magic number (even though it's an obvious one if you have done web application development before).

jdamerow · 2025-01-08T14:38:19Z

concepts/conceptpower.py

+        url = "{0}Concept?id={1}".format(self.endpoint, uri)
+        response = requests.get(url, headers=headers)
+        data = {}
+        if response.status_code == 200:


jdamerow · 2025-01-08T14:41:35Z

concepts/conceptpower.py

+
+    def parse_concept(self,concept_entry):
+        """
+        Parse a concept and return a dictionary with the required fields.


needs parameter description (what does this method accept to get in concept_entry? maybe even put in example json (think about what would someone need to know to use this method if they don't see the code)).

jdamerow · 2025-01-24T15:58:05Z

concepts/conceptpower.py

+
+
+class Conceptpower:
+    # Default behavior is to leave these to the constructor.


what does this mean?

jdamerow · 2025-01-24T15:58:22Z

concepts/conceptpower.py

+    namespace = None
+
+    def __init__(self, **kwargs):
+        # Give first priority to the class definition, if endpoint or namespace are defined (and not None).


what does that mean?

jdamerow · 2025-01-24T16:23:44Z

vogon/settings.py

-CONCEPTPOWER_ENDPOINT = os.environ.get('CONCEPTPOWER_ENDPOINT')
-CONCEPTPOWER_NAMESPACE = os.environ.get('CONCEPTPOWER_NAMESPACE')
+CONCEPTPOWER_ENDPOINT = os.environ.get('CONCEPTPOWER_ENDPOINT', 'http://chps.asu.edu/conceptpower/rest/')
+CONCEPTPOWER_NAMESPACE = os.environ.get('CONCEPTPOWER_NAMESPACE', '{http://www.digitalhps.org/}')


why is the namespace in curly brackets?

this one is fine, it's just a static github page

jdamerow · 2025-01-24T16:24:54Z

vogon/settings.py

@@ -198,8 +198,8 @@

 CONCEPTPOWER_USERID = os.environ.get('CONCEPTPOWER_USERID')
 CONCEPTPOWER_PASSWORD = os.environ.get('CONCEPTPOWER_PASSWORD')
-CONCEPTPOWER_ENDPOINT = os.environ.get('CONCEPTPOWER_ENDPOINT')
-CONCEPTPOWER_NAMESPACE = os.environ.get('CONCEPTPOWER_NAMESPACE')
+CONCEPTPOWER_ENDPOINT = os.environ.get('CONCEPTPOWER_ENDPOINT', 'http://chps.asu.edu/conceptpower/rest/')


and let's not put that url here. even though it is in the github commit history now, we want to keep bot traffic to our servers as low as possible

rajvi-patel-22 added 10 commits December 3, 2024 16:32

[VOGRE-32] Fix bugs in UI for new concept label

595d913

[VOGRE-32] Debug concept lifecycle

10ae7ea

[VOGRE-32] Update conceptpower api

772ed97

[VOGRE-32] Remove approve state and update post request for add

88e3dc0

[VOGRE-32] Addfunction to check equal uris

47ae293

[VOGRE-32] Remove conceptpower api calls

cd3ed73

[VOGRE-32] Code cleanup and fix concept cycle

22c510a

[VOGRE-32] Remove debugging statements

83c6b80

Merge branch 'develop' into story/VOGRE-32

364f396

[VOGRE-32] Remove unnecessary comments

0d3e55a

rajvi-patel-22 requested a review from jdamerow December 16, 2024 22:25

jdamerow requested changes Dec 19, 2024

View reviewed changes

jdamerow closed this Dec 19, 2024

rajvi-patel-22 added 3 commits December 19, 2024 16:32

[VOGRE-32] Create conceptpower class

6b73761

[VOGRE-32] Update conceptpower class and code cleanup

c64ea5b

[VOGRE-32] Fix conceptpower search function

317e7cb

rajvi-patel-22 mentioned this pull request Jan 3, 2025

story/VOGRE-36 #36

Open

9 tasks

rajvi-patel-22 added 3 commits January 3, 2025 15:20

[VOGRE-32] Code cleanup

52110eb

Merge branch 'develop' into story/VOGRE-32

c5a64c4

[VOGRE-32] Remove get_type class method

164e87c

rajvi-patel-22 reopened this Jan 3, 2025

rajvi-patel-22 mentioned this pull request Jan 7, 2025

bug/VOGRE-46 #45

Merged

9 tasks

jdamerow requested changes Jan 8, 2025

View reviewed changes

jdamerow closed this Jan 8, 2025

rajvi-patel-22 added 3 commits January 8, 2025 16:30

[VOGRE-32] Update conceptpower class

1b67101

[VOGRE-32] Fix spacing

4ec9d2d

[VOGRE-32] Add comment for parsing function

dfca435

rajvi-patel-22 reopened this Jan 10, 2025

jdamerow requested changes Jan 24, 2025

View reviewed changes

jdamerow closed this Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Story/VOGRE-32 #33

Story/VOGRE-32 #33

rajvi-patel-22 commented Dec 16, 2024 •

edited

Loading

diging-jenkins commented Dec 16, 2024

jdamerow Dec 19, 2024

jdamerow Dec 19, 2024

jdamerow Dec 19, 2024

jdamerow Dec 19, 2024

jdamerow Dec 19, 2024

jdamerow Jan 8, 2025

jdamerow Jan 8, 2025

jdamerow Jan 8, 2025

jdamerow Jan 8, 2025

jdamerow Jan 8, 2025

jdamerow Jan 8, 2025

jdamerow Jan 24, 2025

jdamerow Jan 24, 2025

jdamerow Jan 24, 2025

jdamerow Jan 24, 2025

jdamerow Jan 24, 2025

		@@ -230,10 +172,16 @@ def merge_with(self, uri):
		children_queryset.update(merged_with=target)

		def add(self):



		class Conceptpower:
		# Default behavior is to leave these to the constructor.

Story/VOGRE-32 #33

Story/VOGRE-32 #33

Conversation

rajvi-patel-22 commented Dec 16, 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?

diging-jenkins commented Dec 16, 2024

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

rajvi-patel-22 commented Dec 16, 2024 •

edited

Loading