Remember flagged state when searching on moderation pages; Fixes #181

[jerone]

Fixes #181

@Zren Is this the solution you had in mind too?

[Zren]

Yep. +1

[Martii]

Hmmm somethings weird here... http://localhost:8080/?library=true&flagged=true&flagged=true&q=jerone brings up a script that isn't normally in the list at http://localhost:8080/?library=true&flagged=true

[Martii]

http://localhost:8080/mod/removed?q=zren brings up nothing but Zren has 11 entries there.

[jerone]

@Martii commented on 7 dec. 2014 18:57 CET:

Hmmm somethings weird here... http://localhost:8080/?library=true&flagged=true&flagged=true&q=jerone brings up a script that isn't normally in the list at http://localhost:8080/?library=true&flagged=true

Because that url will return { flagged: ["true", "true"], ... } and not the expected { flagged: "true", ... }. Not a bug with this PR. Please open an issue if this is an issue for you. Will be an issue for all query items.

Edited

[jerone]

@Martii commented on 7 dec. 2014 19:00 CET:

http://localhost:8080/mod/removed?q=zren brings up nothing but Zren has 11 entries there.

Also not an issue with this PR. Will investigate why that happens.

[Zren]

Hmm. Wonder why i didnt use a hashmap there. Probably because you cant get the key when iterating in mustache. Looks like there needs we need to either check before pushing to that array, use a hahsmap and compile to a list for the view, or delete duplicates before rendering.

[Zren]

Or just store the key inside the value as well as in the key.

[jerone]

@jerone commented on 7 dec. 2014 19:08 CET:

@Martii commented on 7 dec. 2014 19:00 CET:

http://localhost:8080/mod/removed?q=zren brings up nothing but Zren has 11 entries there.

Also not an issue with this PR. Will investigate why that happens.

A found the reason: the column Item is not a searchable property. The column Type is for example a searchable column (http://localhost:8080/mod/removed?q=user) Please open a feature request to add that column to the list.

[Martii]

-1 until the associated issue is created and associated PR is merged along with this one. Remark PR READY when that happens please... this addition is currently useless with no benefit if the whole process doesn't work correctly. e.g. trading one bug for another isn't going to help when searching on moderation pages.

[Martii]

Some steps to reproduce:

• Visit http://localhost:8080/mod
• Visit http://localhost:8080/?flagged=true and http://localhost:8080/?library=true&flagged=true and http://localhost:8080/users?flagged=true all in new tabs
• Visit http://localhost:8080/?library=true in new tab then type in jerone for search which yields http://localhost:8080/?library=true&flagged=true&q=jerone

EDIT:

Shorter steps to reproduce:

• Load http://localhost:8080/?library=true&flagged=true in same tab.
• Refresh that page and the DOM starts duplicating the form variable incrementally each time.

Issue not presented before this patch. The incremental addition of &flagged=true does get worse with more times in the url when it gets executed e.g. in the current tested DOM with this snapshot:

<form method="get" action="/">
  <input type="hidden" value="true" name="library">
  <input type="hidden" value="true" name="flagged">
  <input type="hidden" value="true" name="flagged">
  <input type="hidden" value="true" name="flagged">
  <input type="hidden" value="true" name="flagged">
  <input type="hidden" value="true" name="flagged">
  <div class="input-group col-xs-12">
    <input type="text" value="" class="search form-control" placeholder="Search Libraries" name="q">
    <span class="input-group-btn">
      <button type="submit" class="btn btn-default">
        <i class="fa fa-search"></i>
      </button>
    </span>
  </div>
</form>

[Martii]

either check before pushing to that array

@jerone
This is one of the solutions to the issue that this PR introduces based off and corroborated with from one of @Zren 's comments. You may of course put that routine, modified or not, anywhere in the code you like if anyone doesn't like the positioning. I suspect we might be encountering this at a later date so it might be wise to lib it somewhere:

var applyModelListQueryFlaggedFilter = function (aModelListQuery, aOptions, aFlaggedQuery) {
  var found = null;

  // Only list flagged items if authedUser >= moderator or if authedUser owns the item.
  if (aOptions.isYou || aOptions.isMod) {
    // Mod

    if (aFlaggedQuery) {
      if (aFlaggedQuery === 'true') {
        aOptions.isFlagged = true;

        // Check for an existing name/value prop
        found = false;
        aOptions.searchBarFormHiddenVariables.forEach(function (aProp) {
          if (aProp.name === 'flagged' && aProp.value === 'true') {
            found = true;
          }
        });

        if (!found) {
          aOptions.searchBarFormHiddenVariables.push({ name: 'flagged', value: 'true' });
        }

        aModelListQuery.and({ flags: { $gt: 0 } });
      }
    }
  } else {
    // Hide
    // Script.flagged is undefined by default.
    aModelListQuery.and({ flagged: { $ne: true } });
  }
};

EDIT: This still doesn't address general lib searching though as I mentioned above... so that's still a bug with this PR. e.g aFlaggedQuery seems to be always returning 'true' when this patch is applied, after one search in flagged libs and then searching in all libs.

[Zren]

Use _.findWhere(list, properties) please if that's the fix you choose.

if (!_.findWhere(aOptions.searchBarFormHiddenVariables, { name: 'flagged' }) {
  aOptions.searchBarFormHiddenVariables.push({ name: 'flagged', value: 'true' });
}

Edit: Woops. I had mixed up the properties parameter in the example.

[jerone]

I don't have time for this. Feel free to fix it.

[Martii]

please if that's the fix you choose

Keep forgetting we do have some lib based functions in the underscore package... not a critical point in our code so I don't see an issue with using it instead.

@Zren
Is there a way to pass to the aReq in and test the URI for existence of the flagged=true and then only add this fix by @jerone ?

[Zren]

aFlaggedQuery IS aReq.query.flagged. You're already testing the URI for it in the conditionals already there. Poorly named variable but whatever.

[Martii]

Well I noticed that too but the additional duplicate form parameters are the issue here... the chicken/egg syndrome I believe with that part. After a flagged search then general lib searching adds this patch in which messes up that search so it would need to be not added in.