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

Fix success response code on jpeg images #372

Conversation

YarikMamykin
Copy link
Contributor

@YarikMamykin YarikMamykin commented Jun 4, 2020

Implements/Fixes
#319
#321
#329

This PR is ready for review.

WARNING! This PR should be merged only after #344
Changes in this PR are based on changes from PR 344

Testing Plan

Manual testing

Summary

Enhanced image validation function and added check for template non-png images.

CLA

@YarikMamykin
Copy link
Contributor Author

@theresalech please notice that this PR is ready for Livio Team Code Review.

@@ -274,6 +274,17 @@ SDL.AlertPopUp = Em.ContainerView.create(
// popUp
this.set('priority', priority);
clearTimeout(this.timer);

var callback = function(failed) {
self.reason = 'WARNINGS';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.reason = 'WARNINGS';
self.reason = failed ? 'WARNINGS' : 'SUCCESS';

if(request.params.vrHelp) {
for(var i = 0; i < request.params.vrHelp.length; i++) {
if(request.params.vrHelp[i].image) {
imageList.push(request.params.vrHelp[i].image.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be an isPng check for the images in this function?

@jacobkeeler
Copy link
Contributor

@YarikMamykin also fix merge conflicts

@jacobkeeler jacobkeeler closed this Nov 5, 2020
@jacobkeeler jacobkeeler reopened this Nov 5, 2020
Copy link
Collaborator

@iCollin iCollin left a comment

Choose a reason for hiding this comment

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

should this be case - insensitive?

edit: i’m not reviewing this PR just had this comment saved from taking a look previously

Comment on lines +435 to +437
const img_extension = '.png';
var search_offset = imagePath.lastIndexOf('.');
return imagePath.includes(img_extension, search_offset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const img_extension = '.png';
var search_offset = imagePath.lastIndexOf('.');
return imagePath.includes(img_extension, search_offset);
return imagePath.endsWith('.png');

@AKalinich-Luxoft
Copy link
Contributor

@iCollin @jacobkeeler because of a lot of conflicts and some architectural changes introduced in release 7.0 it makes sense to completely change this fix.
Closed in favor of #462

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.

4 participants