-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the option to raise issue when license submission failed #320
base: python3
Are you sure you want to change the base?
Conversation
@rtgdk @SantiagoTorres Could you both please take a look at it once and lmk if it's fine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits. I think there's some stuff to improve here :)
src/app/utils.py
Outdated
SPDX Online tools repository. | ||
""" | ||
|
||
body = "**1.** Occured At: {0}\n**2.** Made By: {1}\n**3.** Error Message: {2}\n**4.** License Text: {3}\n".format(occured_at, made_by, error_message, license_text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's easier to do this with a multiline string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would recommend to use multiline string
src/app/views.py
Outdated
licenseRequestUrl = os.path.join(serverUrl, reverse('license-requests')[1:], str(licenseRequestId)) | ||
statusCode = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType, matchId, diffUrl, msg) | ||
data['statusCode'] = str(statusCode) | ||
return JsonResponse(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you can simplify the code by moving the return outside of the if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can actually avoid a bunch of repetition by moving other things out of this if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I will do it.
src/app/utils.py
Outdated
title = "Error Report" | ||
payload = {'title' : title, 'body': body} | ||
headers = {'Authorization': 'token ' + token} | ||
url = "https://api.github.com/repos/spdx/spdx-online-tools/issues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking around it seems that this URL is constructed out of the settings on top, we perhaps should follow the pattern they use.
Not fully relevant to this PR, but I would assume you can combine a bunch of things into a single "submit-to-api" function...
src/app/utils.py
Outdated
SPDX Online tools repository. | ||
""" | ||
|
||
body = "**1.** Occured At: {0}\n**2.** Made By: {1}\n**3.** Error Message: {2}\n**4.** License Text: {3}\n".format(occured_at, made_by, error_message, license_text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would recommend to use multiline string
setTimeout(function() { | ||
$("#messages").html(""); | ||
}, 7000); | ||
var errorMessage = 'Error occured while submitting the license. Please contact to [email protected] with SPDX ID for submitting the license. Would you like to submit this problem to github so that the developers can look at it?'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please contact [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have constant variable with these message which can be referenced here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate a bit please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having all these string message in one file and then referencing here in javascript.
src/app/utils.py
Outdated
""" | ||
|
||
body = "**1.** Occured At: {0}\n**2.** Made By: {1}\n**3.** Error Message: {2}\n**4.** License Text: {3}\n".format(occured_at, made_by, error_message, license_text) | ||
title = "Error Report" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets not directly assign a title. Keep a string constant. Similary for url - construct it from other url or keep it as setting in settings.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the url as setting in the settings.py. What about the title of the issue? Should we just kept it blank?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title should included license name/id
return HttpResponse(response,status=500) | ||
ajaxdict['error'] = str(format_exc()) | ||
ajaxdict['inputlicensetext'] = request.POST['text'] | ||
return JsonResponse(ajaxdict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the caller expects "type" and "data" field now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ugtan What about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rtgdk No. I had to use Json response to get the data after making a ajax request in order to give them the ability to choose if they want to submit it or not.
src/app/views.py
Outdated
data['statusCode'] = str(statusCode) | ||
return JsonResponse(data) | ||
|
||
if request.POST.get('error',None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the main part to utils.py
@rtgdk @SantiagoTorres Please take a look now, I have made the modifications. Thanks |
@Ugtan Overall looks good to me, Can you also paste the screenshot/issue created after clicking submit? Left some comment to address, I'll merge it after that. |
@rtgdk I will post the screenshot tomorrow. |
@Ugtan Can you please address the comments and post the screenshot here? |
@Ugtan ping |
Give an option to the license submitter to raise an issue to the SPDX online tools repository when an error has been encountered. This will make it easier to track the errors and fix them asap.