[feature] Added raw id filter #248 #253

niteshsinha17 · 2021-09-14T13:31:00Z

This filter can be used in other modules to add a raw id filter

Checklist

I have read the contributing guidelines.
I have manually tested the proposed changes.
I have written new test cases to avoid regressions. (if necessary)
I have updated the documentation. (e.g. README.rst)
I have added [change!] to commit title to indicate a backward incompatible change. (if required)
I have checked the links added / modified in the documentation.

closes #248

coveralls · 2021-09-14T13:44:07Z

Coverage increased (+0.1%) to 98.582% when pulling 14159be on issues/248-add-raw-id-filter into 497af83 on master.

nemesifier

@niteshsinha17 could you write a test that covers these 2 lines please?
https://coveralls.io/builds/42860654/source?filename=openwisp_utils%2Fadmin_theme%2Ffilter.py#L16
So we can avoid the build failure for coverage decreasing, albeit is small but it's nice to cover all new lines.

pandafy

Good work @niteshsinha17 👍🏼
I tested this on my local machine and the input filter works perfectly.

I noticed some minor issues with code formatting. We can merge this one those are addressed.

pandafy · 2021-09-15T16:46:24Z

openwisp_utils/admin_theme/static/admin/js/ow-filter.js

+    inputFilters.forEach(function (filter){
+      var href = filter.getAttribute('href');
+      var [key, val] = href.substring(1).split('=');
+      if(val.length === 0 && key in qs){
+
+        delete qs[key];
+      }else if(val.length !== 0) {
+        qs[key] = val;
+      }
+    });


Code formatting can be improved here

Suggested change

inputFilters.forEach(function (filter){

var href = filter.getAttribute('href');

var [key, val] = href.substring(1).split('=');

if(val.length === 0 && key in qs){

delete qs[key];

}else if(val.length !== 0) {

qs[key] = val;

}

});

inputFilters.forEach(function (filter) {

var href = filter.getAttribute('href');

var [key, val] = href.substring(1).split('=');

if (val.length === 0 && key in qs) {

delete qs[key];

} else if (val.length !== 0) {

qs[key] = val;

}

});

don't we have style checks for JS code?

It checks only basic formatting.

openwisp_utils/admin_theme/static/admin/js/ow-filter.js

openwisp_utils/admin_theme/static/admin/css/ow-filters.css

openwisp_utils/admin_theme/static/admin/js/ow-filter.js

pandafy · 2021-09-24T10:47:31Z

tests/test_project/admin.py

+    def queryset(self, request, queryset):
+        if self.value() is not None:
+            return queryset.filter(name__icontains=self.value())


Can this be moved to the InputFilter class? Will this be required to be overridden in derived classes?

Yes need to be overridden,

Now we don't need to override this, right? We can just update the class variable.

nemesifier

Thanks @niteshsinha17 @pandafy!

See my comments below for some minor code improvements.

Definitely let's add a mention of this in the README, like at the end of this section: https://github.com/openwisp/openwisp-utils#admin-utilities, a screenshot in the media branch would be highly appreciated: https://github.com/openwisp/openwisp-utils/tree/media/docs.

nemesifier · 2021-09-24T15:23:40Z

openwisp_utils/admin_theme/templates/admin/input_filter.html

+      {% for key, value in all_choice.query_parts %}
+      <input type="hidden" name="{{ key }}" value="{{ value }}" />
+      {% endfor %}
+      <input type="text" class="ow-input-filter-field" title="{{ spec.value | default_if_none:'' }}"value="{{ spec.value | default_if_none:'' }}"


a space is missing here before value.

I recommend breaking the attributes over new lines, eg:

<input type="text" class="...." title="..." value="..." />

nemesifier · 2021-09-24T15:24:14Z

openwisp_utils/admin_theme/templates/admin/input_filter.html

+      <input type="text" class="ow-input-filter-field" title="{{ spec.value | default_if_none:'' }}"value="{{ spec.value | default_if_none:'' }}"
+        name="{{ spec.parameter_name }}" />
+      {% if not all_choice.selected %}
+      <a class='field-clear' title="{% trans 'Clear' %}" href="{{ all_choice.query_string }}">&#x2716; </a>


same here, I would break this line

pandafy

I have done some basic testing. I will testing it with openwisp-controller and check if there are any issues.
The main goal of the filter is done. I have few queries regarding implementation @niteshsinha17.

pandafy · 2021-10-11T14:37:49Z

openwisp_utils/admin_theme/static/admin/js/menu.js

@@ -422,7 +422,6 @@ function initToolTipHandlers() {
    });
  });
 }
-


Is this change intentional?

pandafy · 2021-10-11T14:38:45Z

tests/test_project/admin.py

+    def queryset(self, request, queryset):
+        if self.value() is not None:
+            return queryset.filter(name__icontains=self.value())


Now we don't need to override this, right? We can just update the class variable.

tests/test_project/tests/test_admin.py

openwisp_utils/admin_theme/filter.py

README.rst

openwisp_utils/admin_theme/filter.py

closes #248

pandafy · 2021-11-22T18:59:13Z

openwisp_utils/admin_theme/templates/admin/input_filter.html

+<div class="ow-filter ow-input-filter">
+  <div class="filter-title" title="{{ choice.display }}">
+  {% with choices.0 as all_choice %}
+    <h3>{% blocktrans with filter_title=title %} By {{ filter_title }} {% endblocktrans %}</h3>


Suggested change

<h3>{% blocktrans with filter_title=title %} By {{ filter_title }} {% endblocktrans %}</h3>

<h3>{% blocktrans with filter_title=title %}By {{ filter_title }}{% endblocktrans %}</h3>

nemesifier

I did some testing and looked at the code, it's good to merge except a few minor whitespace issues, see below.

openwisp_utils/admin_theme/static/admin/js/ow-filter.js

nemesifier · 2021-11-29T15:55:26Z

openwisp_utils/admin_theme/static/admin/js/ow-filter.js

+          delete qs[key];
+        }
+
+      }else if(val.length !== 0) {


can we add a space after the }? Isn't the JS linter complaining about this? I thought this would be something to complain about.

nemesifier · 2021-11-29T15:55:54Z

openwisp_utils/admin_theme/templates/admin/filter.html

-      <div id="{{title|join_string}}" aria-label="by {{title}}" tabindex="0"
+  <div  id="{{title|join_string}}"
+        aria-label="by {{title}}"
+        tabindex="0"


I see a few extra white spaces here and in several other HTML files, see my next comments.

nemesifier · 2021-11-29T15:56:02Z

openwisp_utils/admin_theme/templates/admin/filter.html

+      {{ selected_choice | default_if_none:'------' }}
+  </div>
+  </div>
+  <div  id="choices-{{title|join_string}}"


nemesifier · 2021-11-29T15:56:06Z

openwisp_utils/admin_theme/templates/admin/filter.html

-        <a {% if choice.selected %} class="selected"{% endif %} title="{{ choice.display }}"
-        href="{{ choice.query_string|iriencode }}">
-        {{ choice.display }}</a>
+        <a  {% if choice.selected %} class="selected"{% endif %}


nemesifier · 2021-11-29T15:56:10Z

openwisp_utils/admin_theme/templates/admin/input_filter.html

+      {% for key, value in all_choice.query_parts %}
+      <input type="hidden" name="{{ key }}" value="{{ value }}" />
+      {% endfor %}
+      <input  type="text"


niteshsinha17 force-pushed the issues/248-add-raw-id-filter branch 2 times, most recently from b963943 to a40903e Compare September 14, 2021 13:39

nemesifier reviewed Sep 15, 2021

View reviewed changes

pandafy requested changes Sep 15, 2021

View reviewed changes

pandafy assigned niteshsinha17 Sep 15, 2021

pandafy reviewed Sep 24, 2021

View reviewed changes

nemesifier requested changes Sep 24, 2021

View reviewed changes

nemesifier changed the title ~~[enhancement] Added raw id filter #248~~ [feature] Added raw id filter #248 Oct 8, 2021

niteshsinha17 force-pushed the issues/248-add-raw-id-filter branch 2 times, most recently from c225b78 to 3d50c85 Compare October 9, 2021 15:49

pandafy requested changes Oct 13, 2021

View reviewed changes

niteshsinha17 added 3 commits November 19, 2021 23:17

[enhancement] Added raw id filter #248

42d0524

closes #248

[chores] Included test and done minor changes

c3ec6d5

[change] Changed Input Filter

1929160

pandafy force-pushed the issues/248-add-raw-id-filter branch from 3d50c85 to 9783b19 Compare November 19, 2021 18:18

[requested-changes] Added SimpleInputFilter

b102173

pandafy force-pushed the issues/248-add-raw-id-filter branch from 9783b19 to b102173 Compare November 19, 2021 19:01

[requeted-changed] Added missing coverage

cf98fa6

pandafy force-pushed the issues/248-add-raw-id-filter branch from 7d65c88 to cf98fa6 Compare November 19, 2021 20:55

pandafy self-assigned this Nov 22, 2021

pandafy mentioned this pull request Nov 22, 2021

[chores] Added SimpleInputFilter from openwisp_utils.admin_theme openwisp/openwisp-controller#572

Merged

pandafy reviewed Nov 22, 2021

View reviewed changes

[skip ci][chores] Minor changes

bd5d4df

nemesifier requested changes Nov 29, 2021

View reviewed changes

[requested-changes] Removed excess whitespaces

14159be

pandafy force-pushed the issues/248-add-raw-id-filter branch from be5e3c5 to 14159be Compare November 29, 2021 16:39

nemesifier approved these changes Nov 29, 2021

View reviewed changes

nemesifier merged commit 6ecb0be into master Nov 29, 2021

nemesifier deleted the issues/248-add-raw-id-filter branch November 29, 2021 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Added raw id filter #248 #253

[feature] Added raw id filter #248 #253

niteshsinha17 commented Sep 14, 2021

coveralls commented Sep 14, 2021 •

edited

Loading

nemesifier left a comment

pandafy left a comment •

edited

Loading

pandafy Sep 15, 2021

nemesifier Sep 15, 2021

niteshsinha17 Sep 23, 2021 •

edited

Loading

pandafy Sep 24, 2021

niteshsinha17 Oct 5, 2021

pandafy Oct 11, 2021

nemesifier left a comment

nemesifier Sep 24, 2021

nemesifier Sep 24, 2021

pandafy left a comment

pandafy Oct 11, 2021

pandafy Oct 11, 2021

pandafy Nov 22, 2021

nemesifier left a comment

nemesifier Nov 29, 2021

nemesifier Nov 29, 2021

nemesifier Nov 29, 2021

nemesifier Nov 29, 2021

nemesifier Nov 29, 2021

@@ @@ -422,7 +422,6 @@ function initToolTipHandlers() { @@
                   });
                 });
               }

	<h3>{% blocktrans with filter_title=title %} By {{ filter_title }} {% endblocktrans %}</h3>
	<h3>{% blocktrans with filter_title=title %}By {{ filter_title }}{% endblocktrans %}</h3>

[feature] Added raw id filter #248 #253

[feature] Added raw id filter #248 #253

Conversation

niteshsinha17 commented Sep 14, 2021

Checklist

coveralls commented Sep 14, 2021 • edited Loading

nemesifier left a comment

Choose a reason for hiding this comment

pandafy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteshsinha17 Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandafy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 14, 2021 •

edited

Loading

pandafy left a comment •

edited

Loading

niteshsinha17 Sep 23, 2021 •

edited

Loading