Skip to content

Add Ajax#17

Merged
sonus21 merged 31 commits into
sonus21:masterfrom
tboulogne:master
Oct 27, 2020
Merged

Add Ajax#17
sonus21 merged 31 commits into
sonus21:masterfrom
tboulogne:master

Conversation

@tboulogne

Copy link
Copy Markdown
Contributor

Hello,

I'm back with a new contribution :-) :

  • Add ajax to list and navigation
  • Add columns filters :).

Regards

notification_send > for notification
ticket_raise > for ticket creation
Check if a ticket already raised or notification send before sending
- add head_script block for extending script and css
- add header_block
- refactor detail template
- refactor list template
- ad pre tag to prettify request_data and exception detail view
add method to parse cookie
set APP_ERROR_XXX_ONCE to False
rename cookie_parse to parse_headers
- add filters
- start translation sinto templates

@sonus21 sonus21 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test cases as well.

class Migration(migrations.Migration):

dependencies = [
('error_tracker', '0001_initial'),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct, can you please verify if this is working? For me it's failing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i open an issue on it : #16.
Not working for me too...

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you adding error_tracker in the INSTALLED_APPS list?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes...
But with django as name in django_migrations on a previous setup... :-).
I cleanup all, and rerun... no more result...

Note : i need to place error_tracker.django to INSTALLED_APPS to get error_tracker working... and not the documentation specs...

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use error_tracker.DjangoErrorTracker in the installed app and verify?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in new way of declaring modules in django, this line should be présent into init.py of the module

default_app_config = 'error_tracker.apps.DjangoErrorTracker'

I wrote all my modules like this.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please share python and Django version, this error is very strange.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local env (mac OS) :
python : 3.8.5
Django : 3.1.1

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is working fine for me on My Mac and same Django/Python version.
Can you please take master pull and install it via python setup.py install and see if you're still getting similar error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will git it a try

@tboulogne

Copy link
Copy Markdown
Contributor Author

Please add test cases as well.

What test do you expect exactly ? I'm not a pro on test unit... :-)

@sonus21

sonus21 commented Oct 22, 2020

Copy link
Copy Markdown
Owner

Please add test cases as well.

What test do you expect exactly ? I'm not a pro on test unit... :-)

I expect at-least integration tests, see your last merge is failing. Merging and releasing such code is dangerous..

Please validate if your pull requests are at least passing existing tests.
https://travis-ci.org/github/sonus21/error-tracker/pull_requests

@tboulogne

Copy link
Copy Markdown
Contributor Author

not only mine :-)
image

Strange : the failed are commits from my first pull request... not this one.

@sonus21

sonus21 commented Oct 22, 2020

Copy link
Copy Markdown
Owner

not only mine :-)
image

Strange : the failed are commits from my first pull request... not this one.

Oh! Flask optional commit has failed due to the fetch failure., https://travis-ci.org/github/sonus21/error-tracker/builds/732745931

I'm working on to get all test cases pass.

@tboulogne

Copy link
Copy Markdown
Contributor Author

@sonus21 regarding the pull request how could us manage this ? Cancel it and redo it ?

@tboulogne

Copy link
Copy Markdown
Contributor Author

@sonus21 for your eyes ;-)
2020-10-22 21 23 40

Comment thread error_tracker/django/models.py Outdated
@classmethod
def get_exceptions_per_page(cls, page_number=1):
records = cls.objects.all().order_by('last_seen')
def get_exceptions_per_page(cls, query):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**query

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

This is not a kwargs but a true dict i will reuse as filter:

if not query:
            records = cls.objects.all().order_by('last_seen')
 else:
            records = cls.objects.filter(**query).order_by('last_seen')

I will submit a commit with a fix on the jquery in the template. Url parameter are not well formatted when multiple parameter, so this dict, is not well formatted too.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is different from super class(ModelMixin), if someone has implemented via keyword argument page_number. It'll start throwing errors.

That's the reason I'm suggesting this method to take kwargs as parameter. We should not break backward compatibility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let me see how i could manage this...

Comment thread error_tracker/django/utils.py Outdated
import re
import json
from django.http import RawPostDataException
from http.cookies import SimpleCookie

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No usage?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ! yes. A bad copy/paste duplicate this on line 200. On next commit will be fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just submit a fix, let me know.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems http.cookies is not part of python 2.7 and it will fail there, that's the reason it's moved with try except

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i would prefer this on headers

try:
    from http.cookies import SimpleCookie
except:
    pass

instead to see from http.cookies import SimpleCookie inside code.

ok for you ?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But your code would down the line, When you're trying to use SimpleCookie module, you need to add some flag to say this is not available.

fix duplicate SimpleCookie line
optimize jquery script for ajax

error = False
errors = model.get_exceptions_per_page(page_number=page)
errors = model.get_exceptions_per_page(**query)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set page_number if it's missing. And please add **kwargs in ModelMixin, for compatibility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand your meaning :
on page_number

def get_exceptions_per_page(cls, **query):
        if 'page' in query:
            page_number = query['page']
            del query['page']
        else:
            page_number = 1

And please add **kwargs in ModelMixin, for compatibility.

You mean replace in ModelMixin :

@classmethod
@abc.abstractmethod
def get_exceptions_per_page(cls, page_number=1):

with

@classmethod
@abc.abstractmethod
def get_exceptions_per_page(cls, **kwargs):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update model mixin as

@classmethod
@abc.abstractmethod
def get_exceptions_per_page(cls, page_number=1, **kwargs):


Now when we send GET query parameter as dict than we should do something like this

model.get_exceptions_per_page(page_number=page, **query)

I know this will still break the compatibility, so we'll release 2.0.0 to make things fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,

i set in Mixin :

    @classmethod
    @abc.abstractmethod
    def get_exceptions_per_page(cls, page_number=1, **kwargs):
        """
        An object having these properties,
        has_next, next_num, has_prev, prev_num and items
        on the returned object like SQLAlchemy's Pagination
        :return: a paginated object
        """
        raise NotImplementedError

In models.py

    @classmethod
    def get_exceptions_per_page(cls, page_number=1, **query):
        if query and 'page' in query:
            page_number = query['page']
            del query['page']
            
        if not query:
            records = cls.objects.all().order_by('last_seen')
        else:
            records = cls.objects.filter(**query).order_by('last_seen')

Ok for you ?

@sonus21 sonus21 Oct 23, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this works for me, but do you any downside of using **query in filter? For example how it will handle the case of partial match.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

step by step :-). We will start with a full search and study an easy way to make partial search later..
(this later could be there quickly :-) )

@tboulogne tboulogne Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the time is there :-):

Could be manage like this:

query = {"{}__contains".format(k): v for k, v in query.items()}

add page_number to get_exceptions_per_page
add partial text filter
change contains to icontains on query
@tboulogne

Copy link
Copy Markdown
Contributor Author

@sonus21 are we ok on this ?


dependencies = [
('error_tracker', '0001_initial'),
('django', '0001_initial'),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will need to add a mention for updating on README for makemigrations on existing installation.

Comment thread error_tracker/django/utils.py Outdated
try:
from http.cookies import SimpleCookie
try:
from http.cookies import SimpleCookie

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not seem to be at correct place

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a previous conversation you talk about moving this... misunderstanding from my side. Now on headers.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you need to see try/except statement.

try:
from http.cookies import SimpleCookie
// do something with SimpleCookie
except ImportError:
pass

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand your meaning here. Could you explain more here to avoid another misunderstanding please before any change here ?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, when you don't have http.cookies module then you can store cookie data as it as, otherwise we can try to parse cookie data using SimpleCookie module. You can compare this file data with the last commit made by me.

Move Simplecookie to head
@@ -1,23 +0,0 @@
# Generated by Django 3.1.2 on 2020-10-18 11:11

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need this file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you ask previously if we really need it... Will be restored on next commit.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this file but app name is not correct, we can connect over zoom/goole hangout to see what's not working you but same is working for me.

try/except SimpleCookie
@sonus21

sonus21 commented Oct 26, 2020

Copy link
Copy Markdown
Owner

Please fix test issues.

See https://travis-ci.org/github/sonus21/error-tracker/jobs/738936246

@tboulogne

tboulogne commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

Please fix test issues.

See https://travis-ci.org/github/sonus21/error-tracker/jobs/738936246

Running pytest on my side, don't have same errors.

Note : on the CI, lots of errors from flask...

================================================================ ERRORS =================================================================
_______________________________________ ERROR collecting tests/DjangoTest/tests/test_end_point.py _______________________________________
ImportError while importing test module '/Users/XXX/Projets/TNG/error-tracker/tests/DjangoTest/tests/test_end_point.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../.virtualenvs/meubleframe/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
../error-tracker/tests/DjangoTest/tests/test_end_point.py:13: in <module>
    from util import TestBase
E   ModuleNotFoundError: No module named 'util'
=========================================================== warnings summary ============================================================
../../../.virtualenvs/meubleframe/lib/python3.8/distutils/__init__.py:4
  /Users/XXXX/.virtualenvs/meubleframe/lib/python3.8/distutils/__init__.py:4: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/stable/warnings.html
======================================================== short test summary info ========================================================
ERROR ../error-tracker/tests/DjangoTest/tests/test_end_point.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
====================================================== 1 warning, 1 error in 0.90s ======================================================

@sonus21

sonus21 commented Oct 26, 2020

Copy link
Copy Markdown
Owner

Please fix test issues.
See https://travis-ci.org/github/sonus21/error-tracker/jobs/738936246

Running pytest on my side, don't have same errors.

Note : on the CI, lots of errors from flask...

================================================================ ERRORS =================================================================
_______________________________________ ERROR collecting tests/DjangoTest/tests/test_end_point.py _______________________________________
ImportError while importing test module '/Users/XXX/Projets/TNG/error-tracker/tests/DjangoTest/tests/test_end_point.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../.virtualenvs/meubleframe/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
../error-tracker/tests/DjangoTest/tests/test_end_point.py:13: in <module>
    from util import TestBase
E   ModuleNotFoundError: No module named 'util'
=========================================================== warnings summary ============================================================
../../../.virtualenvs/meubleframe/lib/python3.8/distutils/__init__.py:4
  /Users/XXXX/.virtualenvs/meubleframe/lib/python3.8/distutils/__init__.py:4: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/stable/warnings.html
======================================================== short test summary info ========================================================
ERROR ../error-tracker/tests/DjangoTest/tests/test_end_point.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
====================================================== 1 warning, 1 error in 0.90s ======================================================

You should run bash run-tests.sh, this command will report test failures. You can also find via run-tests.sh how does test suite work in error-tracker. It's not using pytests, it's built on basic python test modules.

@tboulogne

Copy link
Copy Markdown
Contributor Author

@sonus21 fix test. No errors on django part on my side. But my fix seems to conflict. Don't know how to solve this.

@sonus21 sonus21 merged commit 4c8193c into sonus21:master Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants