Skip to content
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

Fixing Redirect on Successful License Submission #450

Merged
merged 8 commits into from
Jun 24, 2023

Conversation

BassCoder2808
Copy link
Contributor

Intended to Solve #449

if(githubCode == '201'){
var successMessage = "The license request has been successfully submitted.";
$("#messages").html('<div class="alert alert-success alert-dismissable fade in"><a href="#" class="close" data-dismiss="alert" aria-label="close">&times;</a><strong>Success! </strong>'+ successMessage +'</div>');
window.location.href = "https://github.com/spdx/licnse-list-xml/issues/" + githubIssueId;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think directly transferring users to a new website is ideal. We should either redirect to our own license ID page which should list down the github url (i think it doesn't now) or open this github url in a new tab

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @rtgdk , I had also suggested the same idea like once there is a successful license submission the user should get redirected to 'https://tools.spdx.org/app/license_requests/{id}' wherein they can see more about his license instead of just again redirecting them to submit new license. Discussion but @jlovejoy suggested that currently the working is like it gets redirected to the github page which was not working when I tried it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah opening github link looks fine - both technically and legally. But this should get opened in a new tab. The user should still have a tab to navigate spdx tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will update the PR according so that whenever the new license get's submitted it gets opened in a new tab

src/app/views.py Outdated
@@ -162,7 +163,7 @@ def submitNewLicense(request):
licenseId = licenseRequest.id
serverUrl = request.build_absolute_uri('/')
licenseRequestUrl = os.path.join(serverUrl, reverse('license-requests')[1:], str(licenseId))
statusCode = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see multiple calls to utils.createIssue. You will need to update it elsewhere as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rtgdk , I have made the above changes, please let me know, if anything else needs to be changed in this

src/app/views.py Outdated
@@ -162,7 +163,7 @@ def submitNewLicense(request):
licenseId = licenseRequest.id
serverUrl = request.build_absolute_uri('/')
licenseRequestUrl = os.path.join(serverUrl, reverse('license-requests')[1:], str(licenseId))
statusCode = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1% of developers fix this issue

E501: line too long (235 > 79 characters)

❗❗ 2 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
src/app/views.py 991
src/app/views.py 1166

Visit the Lift Web Console to find more details in your report.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

src/app/views.py Outdated
@@ -162,7 +163,9 @@ def submitNewLicense(request):
licenseId = licenseRequest.id
serverUrl = request.build_absolute_uri('/')
licenseRequestUrl = os.path.join(serverUrl, reverse('license-requests')[1:], str(licenseId))
statusCode = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1% of developers fix this issue

W291: trailing whitespace

❗❗ 5 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
src/app/views.py 167
src/app/views.py 993
src/app/views.py 994
src/app/views.py 1170
src/app/views.py 1171

Visit the Lift Web Console to find more details in your report.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

src/app/views.py Outdated
@@ -162,7 +163,9 @@ def submitNewLicense(request):
licenseId = licenseRequest.id
serverUrl = request.build_absolute_uri('/')
licenseRequestUrl = os.path.join(serverUrl, reverse('license-requests')[1:], str(licenseId))
statusCode = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseExamples, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = utils.createIssue(licenseAuthorName, licenseName, licenseIdentifier,
licenseComments, licenseSourceUrls, licenseHeader,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1% of developers fix this issue

E501: line too long (120 > 79 characters)

❗❗ 7 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
src/app/views.py 168
src/app/views.py 993
src/app/views.py 994
src/app/views.py 995
src/app/views.py 1170
src/app/views.py 1171
src/app/views.py 1172

Visit the Lift Web Console to find more details in your report.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Collaborator

@rtgdk rtgdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BassCoder2808 I see one more use of createIssue in api/views.py. Please make changes, also left a minor comment. Will merge post these.

setTimeout(function() {
$("#messages").html("");
}, 7000);
window.open("https://github.com/spdx/licnse-list-xml/issues/" + githubIssueId, "_blank");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's have this as constant - https://github.com/spdx/licnse-list-xml/issues/

src/app/utils.py Outdated
@@ -352,7 +352,7 @@ def createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseCommen
headers = {'Authorization': 'token ' + token}
url = "{0}/issues".format(TYPE_TO_URL_LICENSE[urlType])
r = requests.post(url, data=json.dumps(payload), headers=headers)
return r.status_code
return r.status_code, r.json()["number"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will r.json() always work without issues? What about when status_code is not as expected like 404, 400, will r.json() parse correctly. Also, will "number" be always in the json? If not let's parse status_code for successfull code list and parse number key if exists. @BassCoder2808

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actually, the number parameter will not be there if there was any issue while creating it through the API, I have added the code to handle that now, let me know if anything else needs to be done in that.

@BassCoder2808 BassCoder2808 requested a review from rtgdk June 8, 2023 17:24
@@ -313,7 +313,7 @@ def submit_license(request):
if 'urlType' in request.POST:
# This is present only when executing submit license via tests
urlType = request.POST["urlType"]
statusCode = createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseRequestUrl, token, urlType)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6% of developers fix this issue

E501: line too long (200 > 79 characters)


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

return None, None


if status_code in [200, 201] and "number" in response_json:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9% of developers fix this issue

E303: too many blank lines (2)


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -313,7 +313,7 @@ def submit_license(request):
if 'urlType' in request.POST:
# This is present only when executing submit license via tests
urlType = request.POST["urlType"]
statusCode = createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseRequestUrl, token, urlType)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18% of developers fix this issue

E1120: No value for argument 'urlType' in function call


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sonatype-lift ignoreall

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ignoreall command is active on this PR, all the existing Lift issues are ignored.

@@ -313,7 +313,7 @@ def submit_license(request):
if 'urlType' in request.POST:
# This is present only when executing submit license via tests
urlType = request.POST["urlType"]
statusCode = createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseRequestUrl, token, urlType)
statusCode, githubIssueId = createIssue(licenseAuthorName, licenseName, licenseIdentifier, licenseComments, licenseSourceUrls, licenseHeader, licenseOsi, licenseRequestUrl, token, urlType)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11% of developers fix this issue

reportGeneralTypeIssues: Argument missing for parameter "urlType"


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@goneall
Copy link
Member

goneall commented Jun 24, 2023

Looks like @rtgdk comment has been resolved - I'll go ahead and merge to include in the next update

@goneall goneall merged commit ee2cfdf into spdx:main Jun 24, 2023
1 check passed
@@ -352,12 +352,15 @@
data: form,
success: function (data) {
var githubCode = data.statusCode;
var githubIssueId = data.issueId;
var githubUrl = "https://github.com/spdx/licnse-list-xml/issues/";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there is a typo here resulting in a 404 on redirect: licnse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @whot for pointing out the error, I will crate a PR to solve this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants