Merge App and Api by creating a common utility for core logic #316

Ugtan · 2021-06-22T23:55:27Z

Fixes #287

The PR adds support for a common utility that contains all the core logic required for the SPDX Online tools. Both App and Api make use of this utility so, we don't have to test the app and api individually. It reduces redundancy and makes it easy to test the modules and maintain them.

rtgdk

Can you post the test case status for these changes?

rtgdk · 2021-06-27T07:43:28Z

src/api/views.py

@@ -20,6 +20,7 @@
 from api.serializers import ValidateSerializer,ConvertSerializer,CompareSerializer,CheckLicenseSerializer,SubmitLicenseSerializer,ValidateSerializerReturn,ConvertSerializerReturn,CompareSerializerReturn,CheckLicenseSerializerReturn,SubmitLicenseSerializerReturn


Don't force push commits. Makes it hard to review changes. We can squash commits while merging

Alright. I won't force push commits from now onwards.

rtgdk · 2021-06-27T07:45:27Z

src/api/views.py

+            httpstatus = output.get('status', None)
+            context_dict = output.get('context', None)
+            response = output.get('response', None)
+            message = output.get('message', None)


Have a common function for this too, as this conversion if needed in every api

Sure. I was thinking the same. It would reduce code redundancy as this piece of code is used both in app and api.

rtgdk · 2021-06-27T07:46:07Z

src/api/views.py

+            elif httpstatus == 400:
+                returnstatus = status.HTTP_400_BAD_REQUEST
+            else:
+                returnstatus = status.HTTP_404_NOT_FOUND


Common function for this conversion too

rtgdk · 2021-06-27T07:46:39Z

src/api/views.py

+            elif response:
+                result = response
+            else:
+                result = message


Can we simplify this logic?

rtgdk · 2021-06-27T07:50:42Z

src/api/views.py

-            query.result=result
-            query.status=httpstatus
-            ValidateFileUpload.objects.filter(file=uploaded_file).update(result=result,status=httpstatus)
+            ValidateFileUpload.objects.filter(file=uploaded_file).update(result=result, status=httpstatus)
            serial = ValidateSerializerReturn(instance=query)
            return Response(
                serial.data, status=returnstatus


Can we other functions apart from views functions like extensionGiven , getFileFormat, etc. to a utils.py file? Basically this file should contain only core logic, other business logic function should be moved to utils function

Sure. I will do that.

rtgdk · 2021-06-27T07:51:51Z

src/app/core.py

+
+
+def license_compare_helper(request):
+    """


Just confirming that you copied this code from app/views.py, right? Coz similar api code was outdated.

Yeah, it's the code from app views.

rtgdk · 2021-06-27T07:52:59Z

src/api/views.py

@@ -20,6 +20,7 @@
 from api.serializers import ValidateSerializer,ConvertSerializer,CompareSerializer,CheckLicenseSerializer,SubmitLicenseSerializer,ValidateSerializerReturn,ConvertSerializerReturn,CompareSerializerReturn,CheckLicenseSerializerReturn,SubmitLicenseSerializerReturn
 from api.oauth import generate_github_access_token,convert_to_auth_token,get_user_from_token
 from app.models import LicenseRequest
+from app.core import initialise_jpype, license_validate_helper, license_check_helper


Import core instead of specific functions. Make it hard to maintain and also better understanding of code flow.

rtgdk · 2021-06-27T07:54:53Z

@Ugtan Looks good in the first iteration. Can you also attach test case run results here after every PR? Just want to make sure we are not opening a new hole in our application.

Ugtan · 2021-06-28T12:35:51Z

@Ugtan Looks good in the first iteration. Can you also attach test case run results here after every PR? Just want to make sure we are not opening a new hole in our application.

@rtgdk Sure, I will attach a screenshot of the API test cases once I complete the other two APIs for compare and convert most probably today itself. And, I'm testing both APP and API to make sure both of them work simultaneously.

Ugtan · 2021-06-30T03:20:26Z

@rtgdk Could you take a look at it once, please? @SantiagoTorres Would definitely appreciate your review as well. thanks

I have merged App and Api code for license Validate, Compare, Convert, and Check License. I will also attach the screenshot of the test cases of all these functions below.

For Validate API

For Convert API

For Compare API

For Check License API(Not passing idk why)

Getting this re-module error not sure why. Will try to fix it.

rtgdk

Great work @Ugtan left some comments to fix. I'll it merge it post that.

rtgdk · 2021-07-04T05:56:48Z

src/app/core.py

+from time import time
+from traceback import format_exc
+from urllib.parse import urljoin
+from app.views import formatToContentType, getFileFormat


Move these functions to utils.py, views.py should just be a wrapper over helper functions.

Alright, I will move these functions to utils.py file

rtgdk · 2021-07-04T05:57:51Z

src/app/core.py

+from time import time
+from traceback import format_exc
+from urllib.parse import urljoin
+from app.views import formatToContentType, getFileFormat


This will be a cyclic dependency kind of logic, views depends on core, core depends on views for some functions

Understood.

rtgdk · 2021-07-04T05:59:18Z

@Ugtan For check license api, it might be failing due to python3 str/bytes issue on spdx_license_matcher side.

rtgdk · 2021-07-04T06:02:34Z

@Ugtan Merged the django3 PR. Please rebase those changes with this PR and post the test results again for both app and api. Also, please make sure that we didn't lose any functions/functionality due to these changes. I'll merge after you post the results post rebase.

Ugtan · 2021-07-04T12:54:17Z

For check license api, it might be failing due to python3 str/bytes issue on spdx_license_matcher side.

I'm not quite sure regarding the issue as it works perfectly when I use the check license functionality on localhost. I also tried to run the SPDX License Matcher and it worked as well. I'm facing the issue only while running the test cases to check license API.

@Ugtan Merged the django3 PR. Please rebase those changes with this PR and post the test results again for both app and api. Also, please make sure that we didn't lose any functions/functionality due to these changes. I'll merge after you post the results post rebase.

I will rebase the branch and share the test results for both the app and API with you. I have briefly tested everything and the test cases also working fine so I don't think we will run into any problem. @rtgdk

rtgdk · 2021-07-08T12:21:47Z

@Ugtan Can you share the test results?

Ugtan · 2021-07-09T04:38:58Z

Here you go @rtgdk

Test results for the API:

The APP test cases are failing after the changes I'm not quite sure why 15 of them failed with the following error:

I'm not quite sure why are these test cases failing. I also tried to dig a little but couldn't figure out the root cause for the error to occur. I have been trying for some time, Could you help if possible? @rtgdk

Ugtan · 2021-07-12T01:40:03Z

@rtgdk Fixed it. Both APP and API test cases are working. I have already shared the API ones. The test results for the app:

rtgdk

@Ugtan Thanks! Merging this.

Ugtan force-pushed the merge_api_app branch 2 times, most recently from 01f9aa3 to 8c66e7a Compare June 26, 2021 23:28

rtgdk requested changes Jun 27, 2021

View reviewed changes

Ugtan requested a review from rtgdk June 30, 2021 03:20

rtgdk reviewed Jul 4, 2021

View reviewed changes

Ugtan added 4 commits July 4, 2021 22:33

Merge APP and API by creating a common utility

5c7d3c9

Fix compare and convert APIs

78271eb

Fix API test cases

1e41593

Remove unnecessary functions and move neccessary once to utils

79225ef

Ugtan force-pushed the merge_api_app branch from 87ca8f1 to 79225ef Compare July 4, 2021 17:04

Move helper functions to app utils file

efc4b95

Ugtan added 2 commits July 9, 2021 10:13

Fix check license API test case

cc8d992

Fix render function args

7ce9bbf

rtgdk approved these changes Jul 16, 2021

View reviewed changes

rtgdk merged commit 220fed0 into spdx:python3 Jul 16, 2021

rtgdk mentioned this pull request Mar 12, 2023

Merge app and api code under a common utils #287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge App and Api by creating a common utility for core logic #316

Merge App and Api by creating a common utility for core logic #316

Ugtan commented Jun 22, 2021

rtgdk left a comment

rtgdk Jun 27, 2021

Ugtan Jun 28, 2021

rtgdk Jun 27, 2021

Ugtan Jun 28, 2021

rtgdk Jun 27, 2021

Ugtan Jun 28, 2021

rtgdk Jun 27, 2021

rtgdk Jun 27, 2021

Ugtan Jun 28, 2021

rtgdk Jun 27, 2021

Ugtan Jun 28, 2021

rtgdk Jun 27, 2021

rtgdk commented Jun 27, 2021

Ugtan commented Jun 28, 2021

Ugtan commented Jun 30, 2021

rtgdk left a comment

rtgdk Jul 4, 2021

Ugtan Jul 4, 2021

rtgdk Jul 4, 2021

Ugtan Jul 4, 2021

rtgdk commented Jul 4, 2021

rtgdk commented Jul 4, 2021

Ugtan commented Jul 4, 2021 •

edited

Loading

rtgdk commented Jul 8, 2021

Ugtan commented Jul 9, 2021

Ugtan commented Jul 12, 2021

rtgdk left a comment

		@@ -20,6 +20,7 @@
		from api.serializers import ValidateSerializer,ConvertSerializer,CompareSerializer,CheckLicenseSerializer,SubmitLicenseSerializer,ValidateSerializerReturn,ConvertSerializerReturn,CompareSerializerReturn,CheckLicenseSerializerReturn,SubmitLicenseSerializerReturn

Merge App and Api by creating a common utility for core logic #316

Merge App and Api by creating a common utility for core logic #316

Conversation

Ugtan commented Jun 22, 2021

rtgdk 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

Choose a reason for hiding this comment

rtgdk commented Jun 27, 2021

Ugtan commented Jun 28, 2021

Ugtan commented Jun 30, 2021

rtgdk 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

rtgdk commented Jul 4, 2021

rtgdk commented Jul 4, 2021

Ugtan commented Jul 4, 2021 • edited Loading

rtgdk commented Jul 8, 2021

Ugtan commented Jul 9, 2021

Ugtan commented Jul 12, 2021

rtgdk left a comment

Choose a reason for hiding this comment

Ugtan commented Jul 4, 2021 •

edited

Loading