openwisp/openwisp-utils

Python and Django utilities shared between different openwisp modules

openwisp-utils's Issues

Move shared logic here

• admin theme
• DependencyLoader and relative tests
• DependencyFinder and relative tests
• TimeStampedEditableModel
• TimeReadonlyMixin, maybe we can rename this to TimeReadonlyAdminMixin
• MultitenantAdminMixin and relative tests
• MultitenantOrgFilter and relative tests
• MultitenantObjectFilter and relative tests

Tests

The tests will be the tricky part, we should ensure some test classes can be reused (the multitenant admin for example), so we move the main test logic to this module and in the other modules we just import the test classes and repeat them there to ensure the integration works correctly, there's an example of this technique in openwisp-controller, which imports the openwisp-users admin tests.

In the case of openwisp-controller, importing the admin tests from openwisp-users was necessary because an integration bug was not spotted during development until it went into production.

[enchancement] Common CI script

We need a common CI script to run all the CI tests, so that if there is minor change or a new check is added/removed, we do not need to update all the repositories with the minor change.

Read more: #37 (comment)

Document OPENWISP_ADMIN_SHOW_USERLINKS_BLOCK setting

Add documentation for OPENWISP_ADMIN_SHOW_USERLINKS_BLOCK settings in README.rst simialar to what has been done for other settings.
`

@nemesisdesign please add corresponding labels : good first issue GCI

Improve QA checks: black, re-format code, commit message check, explain in docs

We've been spending a lot of time on code formatting during reviews lately.

In the past we discussed introducing black, I think we could start introducing it, I leave some notes here:

• add it to the QA requirements
• add it to the QA checks (ensure there's a way to skip the check)
• add a way to trigger the reformatting using isort and black (eg: openwisp-qa-format), so we can format the code with one command (hence making it easy for new contributors)
• update the python code convetions docs to suggest the use of openwisp-qa-format and openwisp-qa-check: http://openwisp.io/docs/developer/contributing.html#python-code-conventions
• add a flag to force the commit message check and add the suggestion of using this feature in the guidelines at: http://openwisp.io/docs/developer/contributing.html#commit-message-style-guidelines

Crash when pending migrations check fails

The follow line contains backticks, making the script to interpret it as a command to be run and fail because manage.py is not expected to be there. See this travis log

openwisp-utils/openwisp-utils-qa-checks

Line 118 in 562e34b

|| { echoerr "ERROR: Migrations check failed! Models' changes not migrated, please run `./manage.py makemigrations` to solve the issue!"; FAILURE=1; }

[deprecated] Remove openwisp-utils-qa-checks in 0.7.0

Remove openwisp-utils-qa-checks in version 0.6.0.

[qa] Add run check migration

GCI Task : link

Override django.contrib.admin.AdminSite instead of modifying it at runtime

The function openwisp_admin() in openwisp_utils.admin_theme.admin changes some settings of the vanilla django admin site at runtime. This makes it difficult to use an overridden admin site instead of the default one together with OpenWISP.

It would be better to override the vanilla admin site instead: https://docs.djangoproject.com/en/2.2/ref/contrib/admin/#customizing-the-adminsite-class Then, for further customization, this overridden class can be overridden once more.

[ci] Newline is not interpreted in openwisp-utils-qa-checks

In the new openwisp-utils-qa-checks, there is a minor bug with the text printed.

openwisp-utils-qa-checks --skip-isort --skip-flake8 --skip-checkmigrations
Running blank endline check
SUCCESS: Blank endline check successful!
Running commit message check
SUCCESS: Commit name check successful!
File manage.py not found\nSkipping Make Migration Check

The last line does not have the newline interpreted correctly. The final result should look like

openwisp-utils-qa-checks --skip-isort --skip-flake8 --skip-checkmigrations
Running blank endline check
SUCCESS: Blank endline check successful!
Running commit message check
SUCCESS: Commit name check successful!
File manage.py not found
Skipping Make Migration Check

[qa] Enforce conventional commit style

Read OpenWISP Commit conventions: openwisp/openwisp-docs#109

I still think we should keep consistency with what we've done until now, so I would use "feature" instead of "feat.
The prefix can be in square brackets, eg: [feature], [change].
Ensure first letter after ] is capital letter.
We should use the same keywords of conventional commits, with the exception of "feat" which is better spelt "feature", 3 characters are worth it in my opinion in this case.
We should also add [change] to the allowed prefixes.

We could then add a commit message template like this: https://thoughtbot.com/blog/better-commit-messages-with-a-gitmessage-template we can try it here first and then replicate it in the other modules.

Maybe we could ditch our own commit check solution in favor of a standard one, I just found this tool which looks really interesting!
https://commitizen-tools.github.io/commitizen/

This change will help us to generate change logs more easily later on.

[rest] Add optional swagger API endpoints

• add extra_requires['rest'] with DRF and swagger in setup.py
• add the URLs for enabling the swagger docs, these URLs will be included in the test project of the other modules
• add a setting named OPENWISP_REST_SWAGGER (or similar) which shall control whether the swagger docs is enabled or not (some users in prod may want to disable this)
• ensure the test coverage does not decrease
• add basic info about this feature in the README

Related to #91.

[admin-theme] menu doesn't respect assigned permissions

The links "Templates" and "VPN Servers" are available in the menu although the user doesn't have the rights for these objects:

I had a look into this a bit:

https://github.com/openwisp/openwisp-utils/blob/master/openwisp_utils/admin_theme/context_processor.py#L32

• ModelAdmin.has_module_permission evaluates permissions only on the module level, not on the model level (Device, Template, Vpn).
• openwisp-utils is decoupled from the other OpenWISP modules so we don't have access to models etc

Add catch_signal test utility

• add catch_signal from openwisp-monitoring to openwisp-utils
• add at least 1 test for it
• add a few lines of docs
• update openwisp-controller and openwisp-monitoring to use that and remove the duplication

[docs] Instructions for development

Instructions for development are redundant. Similar to openwisp/openwisp-users#113 (comment)

In https://github.com/openwisp/openwisp-utils#installing-for-development
Following command does not install dependencies of [qa] and [rest] which are required for running tests.

python setup.py develop

[all-openwisp-repositories] Update the `openwisp-utils-qa-checks`

Thanks to @marfgold1 , now we have #60 merged.

We can now remove multiple checks of openwisp-utils-qa-checks like this:

openwisp-utils-qa-checks --migration-path "./openwisp_users/migrations/ ./tests/testapp/migrations/" --migrations-to-ignore "3 2"

For this issue, you have to implement openwisp-utils-qa-check in all the openwisp repositories that use openwisp-utils-qa-checks multiple times to accomplish the same.

[qa] Post build failure summary information as a comment on github

Whenever there's a build failure in a PR, it would be great to know what has failed and post a summary as a comment in the github PR.
I found a blog post which explains how to do it: https://damien.pobel.fr/post/github-api-from-travisci/
But any other updated online resources which work will be fine.

The comment should only have the relevant information about failures an nothing else.

If tests are failing we could simply say "Some test case have failed".

In case openwisp-qa-check has failed, we could collect the reason of the failure and post it as a message.

One of the ways in which we could do this is to print errors to standard error (instead of printing to standard output), collect the standard error output in a separate var which is then used to build the comment which will be posted to github.

I suggest trying this change in your own fork first (enable travis on your fork and test the failures there).

[enhancement] adding ci test for checking migration name

Add ci test that checks if migration file created has a descriptive name, fail test if default name pattern is found.

[QA] Issue closing not being enforced

The check performed at these lines is not being executed:

openwisp-utils/openwisp_utils/qa.py

Lines 155 to 163 in fc28a78

	if mentions < 1:
	errors.append(
	'You are mentioning an issue in the short description '
	'but it is not clear whether the issue is resolved or not;\n '
	'if it is resolved, please add to the commit long description:\n '
	'"Closes #<issue-number>" or "Fixes #<issue-number>";\n '
	'if the issue is not resolved yet, please use the following form:\n '
	'"Related to #<issue-number>"'
	)

[docs] Document openwisp-utils-qa-check

Add the description of checkendline and openwisp-utils-qa-check to the README, in a similar way to how the other quality assurance scripts are documented.

[all] Centralize all dependencies in openwisp-utils

Let's centralize all the repetitive dependencies in openwisp-utils so we can avoid to repeat them in each package and so that we can manage the dependencies of all the OpenWISP modules.

Dependencies will divide in groups:

• main group, I think this has to be empty, so we can avoid causing non django dependencies to dowload django
• django group, will include the django versions supported by the current version of OpenWISP modules
• rest group, will include django-rest-framework related dependencies, with the versions supported by the current version of the OpenWISP modules
• qa group, this already exists, adding coverage and black here

Then once we have done all these changes, we will remove the dependencies in the other modules and point to the openwisp-utils groups, eg: pip install openwisp-utils[django,rest]>=0.5.0

Document the #noqa option in checkcommit

The commitcheck has an option to avoid running the commitcheck on a particular commit by using the #noqa option.

However, this feature is not documented. please add this with an example in the documentation.

commitcheck --message "[qa] Improved Y #20\n\n Simulation of a special unplanned case\n\n#noqa"

[tests] Increase coverage to 100%

Increase test coverage to 100%.

Move MultitenantAdminMixin to openwisp-users

Reserved for GCI.

Move MultitenantAdminMixin and all similar logic which is related to users and organizations to openwisp-users, also remove openwisp_users from extra_requires in setup.py.

The related unit tests and documentation in README has to be moved as well.

We'll need to increase the version to 0.3.0 because it will be a backward incompatible change

Increase test coverage to atleast 96%

Implement a base serializer class for API views

Django-rest-framework, which has been used to create API views, does not call the full_clean method under models, this means that custom model validations would not run on API views while creating or updating records. We need to create a base serializer class which can be extended to other apps.

[admin-theme] Allow specifying custom CSS and JS

We should add a few settings to the admin_theme module allowing to supply custom CSS and JS files; these should be lists of strings containing URLs to the static files (as users may add multiple files if they need), static files have to be deployed in separate steps (this should be made clear in the README).

Settings could be named:

OPENWISP_ADMIN_THEME_CSS and OPENWISP_ADMIN_JS

As usual we need tests and docs for this feature.

[ci] add check for pending migrations

Add a check to ensure that all the makemigrations are done and there is "No changes detected" in the migrations.

The command to check this would be:

• python manage.py makemigrations --dry-run <module-name> where is optional <module-name> would be passed as an arguement.

[qa] Simplify migration checks

We currently have two migration checks:

• one checks whether there are pending migrations to create (./manage.py makemigrations --dry-run)
• one checks whether the names of the migration files have auto in them

The problem is that we have to specify both --migration-module and --migration-path.
I think we can avoid doing this and have only one command line flag.

I would get rid of --migration-path, keeping only --migration-module, although I would rename this flag to --migration-modules while keeping backward compatibility with the old flag.

--migration-path should be ignored (printing a deprecation warning).

We will need to upgrade the modules using this: https://github.com/search?q=org%3Aopenwisp+%27--migration-path%27&type=Code

[bug] Imcompatibility between isort and black

When running the command: openwisp-qa-check I am seeing an incompatibility between isort and black.

Black wants to convert this to:

-from django_loci.base.models import AbstractFloorPlan, AbstractLocation, AbstractObjectLocation
+from django_loci.base.models import (
+    AbstractFloorPlan,
+    AbstractLocation,
+    AbstractObjectLocation,
+)

While isort keeps changing it to:

+ from django_loci.base.models import AbstractFloorPlan, AbstractLocation, AbstractObjectLocation
- from django_loci.base.models import (
-     AbstractFloorPlan,
-     AbstractLocation,
-     AbstractObjectLocation,
- )

There are many such lines in the django-netjsonconfig.

[bug] Timelogging test runner breaks down if docstring is used in a slow test

I was testing some changes in a test in openwisp_monitoring and thought of using a docstring to better explain a test.
That's when I noticed TimeLoggingTestRunner doesn't work with docstrings, due to one particular line. Below is the traceback, it can be easily reproduced by introducing a docstring in any slow test.

Traceback (most recent call last):
  File "tests/test_project/tests/test_test_utils.py", line 51, in test_time_logging_runner
    runner.run_suite(suite)
  File "/home/hardik/.local/lib/python3.6/site-packages/django/test/runner.py", line 630, in run_suite
    return runner.run(suite)
  File "/usr/lib/python3.6/unittest/runner.py", line 180, in run
    stopTestRun()
  File "/home/hardik/openwisp/openwisp-utils/openwisp_utils/tests.py", line 66, in stopTestRun
    self.display_slow_tests()
  File "/home/hardik/openwisp/openwisp-utils/openwisp_utils/tests.py", line 51, in display_slow_tests
    name, module = name.split()
ValueError: too many values to unpack (expected 2)

Though I haven't seen much usage of docstring in tests in openwisp, still there shouldn't be a limitation, in my opinion. I will try to fix this and submit a patch.

[qa] ReStructuredCheck failing in netjsonconfig, different approach is needed

The check is failing with:

There are some Errors in your RST file syntax 

- <string>:1: (WARNING/2) "raw" directive disabled.

However, I don't think the path we've taken is good.

The repositories which have sphinx documentation already have checks to ensure that documentation compiles and renders successfully, these checks simply generate the sphinx docs and they work reliably.

Now we're mixing things.
I asked to add this check because we need to check that the README files are compatible with pypi.
If we start allowing sphinx syntax in the README, we'll hit again the same issue that some of the sphinx syntax won't work on pypi.

I think the right solution is to separate the two things.

Sphinx documentation checks must be done as we already do: https://github.com/openwisp/netjsonconfig/blob/master/run-qa-checks#L8-L10
This new check should only focus on the README compatibility with pypi, removing the changes we introduced recently to make it work on openwisp-radius.
I would also rename the check to make this more explicit: we can call it checkreadme to ensure things don't get confused.

@KapilBansal320 what do you think?

[requirements] Update dependencies & drop python 2 support

Python2 is very close to the end of it's lifecycle, it's time we drop the support for Python 2.

To do that,

1. We need to update all the dependencies to python3.
2. Remove code that exists for python 2 support.
3. Remove python2 from README & any documentation.

[bug] ReStructuredText failing for sphinx syntax

The ReStructuredText check fails in those repos which are using sphinx, eg: openwisp-radius.

- <string>:43: (ERROR/3) Unknown directive type "toctree".
- 
- .. toctree::
-    :maxdepth: 2
-    :caption: Contents:
- 
-    /developer/setup
-    /developer/freeradius
-    /user/settings
-    /user/management_commands
-    /user/importing_users
-    /user/generating_users
-    /user/enforcing_limits
-    /user/registration
-    /user/social_login
-    /user/api
-    /developer/how_to_extend
-    /developer/contributing
-    /general/goals

@KapilBansal320 can we selectively disable some errors?

[qa] Commit check, allow release commits

Don't fail the commit check if the commit starts with: <numbers dots and letters> release.

See how this PR is failing: openwisp/django-netjsonconfig#146

[menu] Add way to register menu items

Right now, to add menu items in the several openwisp modules, we require a lot of repetition of code:

https://github.com/openwisp/openwisp-users/blob/1e31ea5f4b3a92c1e3c4f783f89fd204a90f23a6/openwisp_users/apps.py#L13-L25

We should simplify this, allowing to register new items with a function, optionally allowing to specify the position at which the items will be added in the menu list (append by default to the end, allow to insert after a specific app or model).

Increasing the font size of admin UI

I think only the header part font is small in vision. Please give me some suggestions

Below is a screenshot of admin ui:

[deps] Add support for django 3.1

Add support for django 3.1.

[qa] Rename openwisp-utils-qa-checks to openwisp-qa-check

For consistency with the new openwisp-qa-format, let's rename openwisp-utils-qa-checks to openwisp-qa-check but we have to maintain backward compatibility with the old command (openwisp-utils-qa-checks) to avoid breaking builds.

[qa] Move ReadOnlyAdmin to openwisp-utils

See openwisp/django-freeradius#201

Generalize easy to copy uuid, secret key and secret URL logic

Reserved for GSoC Students

We have different parts of different OpenWISP modules which have a key or secret field which has one or more of the following features:

• the key is automatically and randomly generated
  • django-netjsonconfig
  • django-netjsongraph
  • openwisp-radius
  • openwisp-controller
• the uuid field in the admin is readonly and can be copied easily:
  • django-netjsonconfig (look for uuid and look at the javascript file)
  • openwisp-users (look for uuid and look at the javascript file)
• the key is used to build a URL which can be copied easily:
  • django-netjsongraph (look for receive_url and then find the receive-url.js javascript)

We need to:

• remove the duplication of this logic across modules and make it reusable
• add automated tests for this logic
• document its usage briefly in the README of this module
• update the other modules to use the generalized solution here in openwisp-utils

[qa] Add ReStructuredText checks to openwisp-qa-check

When publishing to pypi, sometimes the publishing fails because of an error in the README.rst, this may happen once the commit and git tag have been already sent, so it's not ideal.

We should avoid this, it shouldn't be hard:

• we can add it to the qa dependencies this tool https://github.com/mgedmin/restview/
• we can add to the qa check a function that calls the rest check (with possibility to skip this), as follows:
  • if README.rst exists, run the check on it
  • if docs/source exists, run the check recursively on it

This would allow to enable this check on all the repos which make use of openwisp-utils!

[ci] Improve checkmigrations in openwisp-utils-qa-checks

For now, checkmigrations in the new openwisp-utils-qa-checks common CI script is supported only for one migration path and one migration-to-ignore.
In some repo (like django-ipam, django-loci, and more) it's required to check on two migration-path.
And even some repo (like openwisp-users) it's required to check on two migration-path and two different migrations-to-ignore.
To deal with this, we can:

1. Still using the custom runcheckmigration in those repo and use openwisp-utils-qa-checks with --skip-checkmigration. The disadvantage is, well, why should checkmigration exist in the common CI test then... 😅
2. The 'hacky' ways by using two lines of openwisp-utils-qa-checks, with the second call to check for other checkmigration and skip all tests. The disadvantage is when there is an addition/removal in the CI test, we should change those repositories too.

Either ways are fine, but kind of defeated the purpose of openwisp-utils-qa-checks that simplify the workflow of minor change in the CI tests without having to deal with all repositories. So I suggest that we might support multiple checkmigration, separated by semicolon. For example, we can do this for openwisp-users

openwisp-utils-qa-checks --migration-path "./openwisp_users/migrations/;./tests/testapp/migrations/" --migrations-to-ignore "3;2"

Feel free to discuss/suggest/give your opinion about the better ways to do this 😄

Improve reusability of DependencyLoader and DependencyFinder

DependencyLoader and DependencyFinder should allow django to reuse templates and static files of third party apps that are not loaded in settings.INSTALLED_APPS.

Because the original code was tied to openwisp-controller, the two classes now don't make much sense because they will go to look into openwisp_utils.__dependencies__.

What about creating a new setting like EXTENDED_APPS?

In the test suite you could add a dependency to django-netjsonconfig, then set in the test project:

EXTENDED_APPS = ['django-netjsonconfig']

Then you could write 2 tests in which you just instantiate the 2 classes and check the list they produce is correct.

[docs] Content mismatch in Dependency Loader

In the README, text describing Dependency Loader has code for Dependency Finder.

[qa] Add a check for empty line at the end of a file

We should add another automated test to check if all the files have an empty line at the end of the file.
Many GCI students are making this mistake and an automated test will help them in my opinion! 😄

PS: binary files must be ignored, the check should be performed only on text files.
If it can be performed with just a series of shell commands, we can use that and start adding this check to the .travis.yml build of this repo

[qa] Commit check doesn't seem to work anymore on pull requests

The commit used in #126 should cause the check to fail, but it seems the check is not being run.

[tests] Create test utility to redirect output

Recently some contributors have been confused by the fact that some tests print output to standard output and/or standard error, so they thought tests failed when tests are passing.

Let's use two context managers from the standard library to create two decorators that we'll use to decorate the test methods which produce output, the context managers are:

• contextlib.redirect_stdout
• contextlib.redirect_stdout

It should be possible to decorate test methods like the following:

import sys
from openwisp_utils.tests import redirect_stdout, redirect_stderr, redirect_any_output

@redirect_stdout
def test_something_out(self):
    print('this test prints output')

@redirect_stderr
def test_something_err(self):
    print('this test prints some error', file=sys.stderr)

@redirect_any_output
def test_something_out(self):
    print('this test prints output')
    print('this test prints some error', file=sys.stderr)    

Sometimes we also need to check the output, so we need a way to access it, eg:

@redirect_stdout(io.StringIO())
def test_something_out(self, output):
    action_generating_output()  # pseudocode
    self.assertIn('expected output', output.getvalue())

@redirect_any_output(stdout=io.StringIO(), stderr=io.StringIO())
def test_something_out(self, stdout, stderr):
    action_generating_output()  # pseudocode
    self.assertIn('expected stdout', stdout.getvalue()) 
    self.assertIn('expected stderr', stderr.getvalue())  

Here's a sample decorator which does not use the context managers above but which may adapted to do so: https://gist.github.com/jasonbartz/8480004

As a last step, use this decorator to silence the output of some tests in this module.

As usual, tests and (brief) docs are needed as well.

Allow overriding the default argument of KeyField

Allow overriding the default argument of KeyField. Would be good to have the get_random_key bound to the KeyField for easier generation of the default value. whenever it needs to be overridden.

[qa] Add commit message style check

It's time to automate the commit message style checks according to our guidelines, that will make contributing a lot smoother for new contributors and less time consuming for reviewers.

These are the most common issues which we should address:

• prefix not present, eg: Done something (should be [qa] Done something)
• no capital letter after prefix: [qa] done something
• unnecessary final dot in first line, eg: [qa] Done something.
• No blank line before first line and second line, eg:

[qa] Done something
Long description

• Mentions an issue without closing it, eg: Implements #20 (won't close), Closes #20 and #21 (won't close #21 - but we should allow writing something like Related to #33, because sometimes we may want to reference an issue even though we are not solving it in that commit
• Closes an issue in the long description but doesn't mention it in the short description, eg:

[qa] Done something

Fixes #2

should be:

[qa] Done something #2

Fixes #2

(easier to read in the short log format)

This is not urgent but not unimportant because having a clean commit history is important for the long term sustainability of the project.
It doesn't make sense to keep enforcing these rules by hand.. it's clear to me that as the community grows we won't be able to do that, so automating this check is key.

We need something like the migration check implemented in #16.

Improve instructions for installation during development