Added warnings UI

[arpan98]

Tries to solve #29 Changed the warnings data structure to correspond to each run individually. Had to implement a slight workaround by saving the temporary data because like here, updateRuns() is called after the entire run process has been ended. Also made a tooltip on a exclamation mark beside the run name to show the number of warnings. I was unable to find more than one log with warnings, so I have not yet tested the individual-run-warnings feature. Please link any logs you find with warnings. Also, need some details as to what UI is to be shown when the exclamation mark is clicked. Have a look @martiansideofthemoon @jgraham

---

This change is

[martiansideofthemoon]

The fix fails when you have > 1 log file. Try this https://drive.google.com/file/d/0B5Y_SiDYwIOba2FYd25TQ1N3NmM/view?usp=sharing

[arpan98]

The problem arises because the warnings list is not committed to the database. So, every time getRuns command is passed, the data is received from the database again and the warnings list is lost. Hence, on adding multiple log files, all warnings of previously imported files are lost.

Any suggestions on what to do here? @martiansideofthemoon @jgraham We could save the warnings list to the database as a JSON string, that is one way.

[jgraham]

Yeah, if we want to display these, they should be saved to the database in some way. I guess given the relative slowness of LoveField, just saving a single string might make more sense than creating a warnings table, which would be my natural instinct here.

[arpan98]

The string would be quite large though, for example the log file linked by @martiansideofthemoon has 570 warnings, that is, 570 duplicate objects.

[jgraham]

I'm not too worried about it being large since the data here is generally rather large.

[arpan98]

@jgraham @martiansideofthemoon Have a look at the fix. It works for multiple runs. Need some guidelines for the UI for displaying the warnings.

[martiansideofthemoon]

Good effort! I couldn't get it to work due to the error mentioned though :cry:

Previously, arpan98 (Arpan Banerjee) wrote…

> @jgraham @martiansideofthemoon Have a look at the fix. It works for multiple runs. Need some guidelines for the UI for displaying the warnings. >

---

Reviewed 1 of 3 files at r1, 3 of 3 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

_angular_scripts.js, line 266 [r2] (raw file):_

          run.edit = false;
          run.newName = "";
          run.warnings = JSON.parse(run.warnings);

There seems to be a problem here as run.warnings is undefined in this query string.

---

Comments from Reviewable

[martiansideofthemoon]

Sorry my bad, didn't delete the old database. Perhaps a user could click on the warnings icon and get a list of warnings in the bottom panel we have? (https://github.com/mozilla/wptview/blob/master/index.html#L161)

Previously, martiansideofthemoon (Kalpesh Krishna) wrote…

> Good effort! I couldn't get it to work due to the error mentioned though :cry: >

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

_angular_scripts.js, line 332 [r2] (raw file):_

  function saveWarnings(runName, duplicates) {
    return resultsModel.updateWarnings(runName, duplicates)
      .then(() => {console.log("Updated warnings.");});

You don't need a saveWarnings function.

---

_Comments from Reviewable_

[martiansideofthemoon]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

wptview.css, line 121 [r2] (raw file):

    padding: 5px 0;
    border-radius: 6px;

remove this space

---

Comments from Reviewable

[arpan98]

On clicking the icon, I'm showing the list in the detailsPanel. Have a look @jgraham @martiansideofthemoon

[jgraham]

Reviewed 1 of 3 files at r2, 3 of 4 files at r5. Review status: 4 of 5 files reviewed at latest revision, 11 unresolved discussions.

---

_angular_scripts.js, line 288 [r5] (raw file):_

    .then((duplicates) => {
      return resultsModel.updateWarnings(name, duplicates)
      .then(() => {console.log("Updated warnings.");});

Don't need the console.log here.

---

_angular_scripts.js, line 553 [r5] (raw file):_

  $scope.showWarnings = function(run) {
    $scope.displayWarnings.runName = run.name;
    $scope.displayWarnings.warnings = angular.copy(run.warnings);

Why is making a copy required?

---

_index.html, line 25 [r5] (raw file):_

            <th></th>
            <th>Run Name</th>
            <th ng-show="runs | warningFilter"></th>

I would make this the last column and show it always.

---

index.html, line 41 [r5] (raw file):

            <td>
              <div class="tooltip" ng-show="run.warnings.length">
                <img ng-style="{'cursor': 'pointer'}" ng-click="showWarnings(run)" src="warning.svg">

Why not just use <img title="{{ run.warnings.length }} warnings" src=…?

---

index.html, line 166 [r5] (raw file):

  </div>
  <div ng-if="displayWarnings.visible" class="detailsPanel">
    <h2> Warnings in {{displayWarnings.runName}} </h2>

I think you should point out that these are duplicate [sub]test names.

---

index.html, line 167 [r5] (raw file):

  <div ng-if="displayWarnings.visible" class="detailsPanel">
    <h2> Warnings in {{displayWarnings.runName}} </h2>
    <ol style="margin-left: 3em; display: list-item; text-indent: -3em;" ng-repeat="warning in displayWarnings.warnings">

Why the inline style?

---

index.html, line 168 [r5] (raw file):

    <h2> Warnings in {{displayWarnings.runName}} </h2>
    <ol style="margin-left: 3em; display: list-item; text-indent: -3em;" ng-repeat="warning in displayWarnings.warnings">
      <div><b>Test</b> : {{ warning.test }}</div>

Shouldn't an ol contain li elements, not divs?

---

index.html, line 170 [r5] (raw file):

      <div><b>Test</b> : {{ warning.test }}</div>
      <div><b>Subtest</b> : {{ warning.subtest }}</div>
      <br />

Why the br?

---

wptview.css, line 113 [r5] (raw file):

}

/* Tooltip text */

I think this tooltip stuff is unneeded.

---

Comments from Reviewable

[arpan98]

Integrated your suggested changes @jgraham . I'll need some help on bringing the detailsPanel down. Currently it just stays up until the page is refreshed. I had a look at #95 but couldn't understand what exactly allowed the panel to close when clicked elsewhere.

[jgraham]

Reviewed 3 of 3 files at r6. Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

index.html, line 165 [r6] (raw file):

    <h2> Duplicate tests in {{displayWarnings.runName}} </h2>
    <ol>
    <li style="margin-top: 0.8em;" ng-repeat="warning in displayWarnings.warnings">

Would be nice to put this style in a stylesheet

---

Comments from Reviewable

[arpan98]

Status?

[jgraham]

There are still open issues in the review.

[arpan98]

Solved the issues with inline CSS.

[jgraham]

Reviewed 2 of 2 files at r7. Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

wptview.css, line 51 [r7] (raw file):

}

.detailsPanel > li {

That doesn't work since the li isn't a child of <div class=detailsPanel>

---

Comments from Reviewable

[arpan98]

Oops, trivial error. Didn't check sorry.

[jgraham]

Reviewed 1 of 1 files at r8. Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[jgraham]

Ah, dammit I clicked on the button rather than the dropdown to select "squash and merge".

Anyway, thanks for the patch!

[martiansideofthemoon]

@jgraham we shouldn't squash it in the master history right?