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

[READY] Fixed issue #15532: Show warnings when implicit alphabetical compare #1341

Merged
merged 19 commits into from Nov 11, 2019

Conversation

@Shnoulle
Copy link
Collaborator

Shnoulle commented Nov 7, 2019

Fixed issue #15532: Show warnings when implicit alphabetical compare
Dev: Move success to alert-success : no issue is a success :)

Still to do

  • Add information on logic file
  • Add test

#1340

With alternative class
Capture d’écran du 2019-11-07 17-44-29

Shnoulle added 5 commits Nov 7, 2019
…is used in expressions

Dev: currently only warning checked
Dev: some string must be reviewed
Dev: still issue with question text ???
@Shnoulle Shnoulle requested a review from olleharstedt Nov 7, 2019
@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Nov 7, 2019

Awesome, will check tomorrow! Now birthday beer for Patrick. :D

Shnoulle added 2 commits Nov 7, 2019
Dev: unicity for warnings string
@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 7, 2019

No warning for sub question relevance : need to be in $sq … arg …

Shnoulle added 2 commits Nov 7, 2019
Dev: Fix #15545 and #15547 for warnings only
Dev: fix some language string
@Shnoulle Shnoulle changed the title Warning bad js alert [WIP] Fixed issue #15532: Show warnings when implicit alphabetical compare Nov 8, 2019
Copy link
Contributor

olleharstedt left a comment

Looks good! Will test.

application/helpers/expressions/em_core_helper.php Outdated Show resolved Hide resolved
@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Nov 8, 2019

Hey, I think we need a warning message here if there are any strange comparisons:

image

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Nov 8, 2019

The link to the manual needs to open manual in new tab.

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 8, 2019

I remove the _blank ?

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 8, 2019

The link to the manual needs to open manual in new tab.

I lkie to add a rel="manual" , and a JS for such link. But merging make it more complicated …

Just add _blank … for master, review for develop

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Nov 8, 2019

Just add _blank … for master, review for develop

Target _blank is perfectly fine.

Shnoulle added 2 commits Nov 8, 2019
Dev: Move success to alert-success : no issue is a success :)
Dev: target=_blank not _target=blank …
@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 8, 2019

Target _blank is perfectly fine.

Yes but not _target = "blank" it don't work … 🤐

Shnoulle added 2 commits Nov 8, 2019
Dev: issue with broken : testCheckNoIssue
@Shnoulle Shnoulle changed the title [WIP] Fixed issue #15532: Show warnings when implicit alphabetical compare [READY] Fixed issue #15532: Show warnings when implicit alphabetical compare Nov 8, 2019
@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 8, 2019

For manual test : 2 ways,

  1. just enter in any string (group description for example ) {1 == "1"} forced alphabetical compare.
  2. Import test survey, check logic file of survey
@Shnoulle Shnoulle requested a review from olleharstedt Nov 8, 2019
… again and again …
$sWarningsText .= "<strong class=''>".$LEM->ngT("This question has at least {n} warning.|This question has at least {n} warnings.",count($aWarnings))."</strong>";
$sWarningsText .= "<ul class='list-unstyled small text-warning'>";
$warningsDone = array();
foreach($aWarnings as $aWarning) {

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

I'm not too happy about this piece of code. If you'd done OOP, this loop would only contain $warning->__toString() and not HTML. And RDP_Warnings could be a Collection object, also using the __toString magic method.

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

But I think I'll merge it today anyway, and release. We can add some @todo and maybe fix it later.

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Nov 11, 2019

Author Collaborator

Yes, sure … but all of this function already do a complete HTML …

}
//~ switch ($key)

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

In the future, please do not mix clean-up with new features in one PR. It makes it harder to read the PR in full. I know, it's hard. :)

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Nov 11, 2019

Author Collaborator

Hard to do new feature without cleaning up a little …

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

Yeah, takes discipline. But clean-up can be done without PR (mostly).

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Nov 11, 2019

Author Collaborator

Yes … totally right … sorry … (and clean up master => merge in develop directlty too …)

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

👍

foreach($aWarnings as $aWarning) {
if(!in_array($aWarning[0],$warningsDone) ) {
$sWarningsText .= "<li>";
if(!empty($aWarning[2])) {

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

Here's a classic if-statement that could be replaced with inheritance.

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Nov 11, 2019

Author Collaborator

. Unsure : warnings can have a link or can not have a link :) It's still need to check if link is OK or not.

Here : we can use null !== $aWarning[2] because it set as null by default.

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Nov 11, 2019

Contributor

Yeah, you're right. Bad example from me. I've kept this logic in my OOP refactoring (see other commit).

@olleharstedt olleharstedt merged commit f9a62f8 into LimeSurvey:master Nov 11, 2019
3 checks passed
3 checks passed
CodeFactor 19 issues fixed. 17 issues found.
Details
Scrutinizer Analysis: 3 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Nov 11, 2019

We still miss floatval as an EM function, btw. Easy to add?

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 11, 2019

Really easy to add, just add it ;) , i can do it . Remind the js function :)

PS : i think of a way to move all current EM function to an external system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.