Rendering and notification field by tboulogne · Pull Request #14 · sonus21/error-tracker

tboulogne · 2020-10-18T20:07:14Z

Hello,

Thanks for your works. With this pull, i'd like to contribute with :

switch to bootstrap 4
two new fields (notification_send, ticket_raised) to avoid multiple notifications or tickets for same error
prettify of request_data
new rendering of details and list (bootstrap 4)
two new blocks : head_script and header_block in template

Feel free to accept what you want.

Regards

Thierry

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

sonus21 · 2020-10-19T02:57:55Z

Just a high level comment from me,
We should take notification send as an additional configuration, some users might want to send notifications for subsequent failures as well but other might not be like you.

Even when we're not sending notifications, we should consider the time difference between two consecutive failures. If we're seeing time difference is greater than some delta than we should send notification, reason for this is we keep overriding the failure, that does not mean yesterday failure is same as today's failure, it could be as well but we're not sure only developer has the right knowledge about such things.

tboulogne · 2020-10-19T04:47:30Z

Hello I will modify this and resend request ;-) Thierry BOULOGNE 06 43 18 31 19

…

Le 19 oct. 2020 à 04:58, Sonu Kumar ***@***.***> a écrit : Just a high level comment from me, We should take notification send as an additional configuration, some users might want to send notifications for subsequent failures as well but other might not be like you. Even when we're not sending notifications, we should consider the time difference between two consecutive failures. If we're seeing time difference is greater than some delta than we should send notification, reason for this is we keep overriding the failure, that does not mean yesterday failure is same as today's failure, it could be as well but we're not sure only developer has the right knowledge about such things. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

tboulogne · 2020-10-19T05:55:57Z

@sonus21 add parameter for notification and ticket.

sonus21 · 2020-10-19T16:26:38Z

@@ -0,0 +1,28 @@
+# Generated by Django 3.1.2 on 2020-10-18 11:17


We don't need this file

sonus21 · 2020-10-19T16:27:21Z

+    ]
+
+    operations = [
+        # migrations.AddField(


We would need to add migrations for exceptions table. And please remove alter for request_data field.

tboulogne · 2020-10-19T16:31:22Z

@sonus21 let me know if better now.

sonus21 · 2020-10-19T16:31:22Z

            try:
-                headers = request.headers.dict()
-            except AttributeError:
+                headers = request.headers


maybe you can pull this to another method to pull cookie.

@sonus21 don't understand your meaning, could you explain a little bit please ?

You can add a function that will get cookie from request object.

sonus21 · 2020-10-19T16:32:16Z

+    notification_send = None
+    ticket_raised = None


Default value can be false.

add method to parse cookie

tboulogne · 2020-10-19T16:50:42Z

@sonus21 let me know if better now please.

sonus21

Overall looks good, there're just few nitpicks, once addressed I'll approve and release a new version.

sonus21 · 2020-10-20T02:23:29Z

    created_on = models.DateTimeField(auto_now=True)
    last_seen = models.DateTimeField(auto_now=True, db_index=True)
-
+    notification_send = models.BooleanField(default=False)


Please rename this to notification_sent

sonus21 · 2020-10-20T02:23:52Z

+    operations = [
+        migrations.AddField(
+            model_name='errormodel',
+            name='notification_send',


notification_sent as column name

sonus21 · 2020-10-20T02:24:24Z

 # Check error views are visible to others or not
 APP_ERROR_VIEW_PERMISSION = get('APP_ERROR_VIEW_PERMISSION', None)
+# Send email notification once
+APP_ERROR_NOTIFICATION_ONCE = get('APP_ERROR_NOTIFICATION_ONCE', None)


Here default should be False

sonus21 · 2020-10-20T02:24:31Z

+# Send email notification once
+APP_ERROR_NOTIFICATION_ONCE = get('APP_ERROR_NOTIFICATION_ONCE', None)
+# Raise ticket once
+APP_ERROR_TICKET_ONCE = get('APP_ERROR_NOTIFICATION_ONCE', None)


sonus21 · 2020-10-20T02:37:13Z

+    for key, value in headers.items():
+        try:
+            # Test if value could be json loaded, parse if needed as for cookie.
+            json.loads('{"%s":"%s"}' % (key, value))


No usage of json.loads?

Emulate json.loads to detect problematic values and set them to value=""

Can you please provide some more details how it helps you and on what value? As you're encoding value in string something like this

{ "foo" : "<value>" }

As your code, any value could be parsed.

set APP_ERROR_XXX_ONCE to False

sonus21 · 2020-10-20T13:39:46Z

+    return x
+
+
+def cookie_parse(headers):


One last change request, this method does not seem to parse only cookie, it's parsing header. Can we rename this method to parse_headers

sonus21 · 2020-10-20T13:53:13Z

+    for key, value in headers.items():
+        try:
+            # Test if value could be json loaded, parse if needed as for cookie.
+            json.loads('{"%s":"%s"}' % (key, value))


Can you please provide some more details how it helps you and on what value? As you're encoding value in string something like this

{ "foo" : "<value>" }

As your code, any value could be parsed.

rename cookie_parse to parse_headers

tboulogne · 2020-10-20T14:09:42Z

json.loads('{"%s":"%s"}' % (key, value))

Parse each key, value from headers items and Test if could be "json loaded". If not, we set the correspondant value to empty except for cookie key.

because, if could not load a key, value with json, pretty display will not works...

hope it help you to understand...

Regards

sonus21 · 2020-10-20T16:41:56Z

json.loads('{"%s":"%s"}' % (key, value))

Parse each key, value from headers items and Test if could be "json loaded". If not, we set the correspondant value to empty except for cookie key.

because, if could not load a key, value with json, pretty display will not works...

How are we going to handle form data? I'm still not convinced this will be able to catch the json load error. Can you please share some example request data?

tboulogne · 2020-10-20T16:44:04Z

@sonus21 could you trust me on this point :-) ? I faced the problem to provide this tortuous solution :-).

tboulogne · 2020-10-20T16:52:03Z

json.loads('{"%s":"%s"}' % (key, value))
Parse each key, value from headers items and Test if could be "json loaded". If not, we set the correspondant value to empty except for cookie key.
because, if could not load a key, value with json, pretty display will not works...

How are we going to handle form data? I'm still not convinced this will be able to catch the json load error. Can you please share some example request data?
Something like that

{'headers': {'Host': 'xxxxxxx', 'Accept': 'text/css,*/*;q=0.1', 'Connection': 'keep-alive', 'Cookie': {'sessionid': 'z027lpgvxo3pf85u6pkzsqdcs87cyojr', 'csrftoken': 'XXXXXXXXXXXXXXXXXXXXXXX', 'messages': '__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.],[__json_message,0,25,Vous \\u00eates d\\u00e9connect\\u00e9.],[__json_message,0,25,Connexion avec myuser r\\u00e9ussie.:1kUbGk:y1uTcIVlaWEYbyJGMKZ5r5qisGggfuA4L6Pt2kflF4w'}, 'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15', 'Accept-Language': 'fr-fr', 'Referer': 'http://127.0.0.1:8000/render_list/products_manager/manageproduct_list/_mp_import_status/ALL/', 'Accept-Encoding': 'gzip, deflate'}}```

sonus21 · 2020-10-21T14:57:42Z

@sonus21 could you trust me on this point :-) ? I faced the problem to provide this tortuous solution :-).

Approved still I need to see.

tboulogne added 10 commits October 18, 2020 08:53

Add field to prevent multiple notifications

4e7bd19

notification_send > for notification ticket_raise > for ticket creation

Check previous send

c58bf62

Check if a ticket already raised or notification send before sending

fix type

225c4ea

update fields

14a376a

save notification send confirm

f943cc2

add returnt obj to create_or_update_entity

f746ec6

add login_required to permission wrapper

ce18743

Pretiffy request_data display

51bf232

- upgrade to bootstrap 4

ace6530

- 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

remove border

f8d5f23

add APP_ERROR_NOTIFICATION_ONCE and APP_ERROR_TICKET_ONCE

eaadf4c

sonus21 reviewed Oct 19, 2020

View reviewed changes

fix migrations

4321131

sonus21 reviewed Oct 19, 2020

View reviewed changes

Set default to NONE

5d10afd

add method to parse cookie

sonus21 requested changes Oct 20, 2020

View reviewed changes

change notification_send to notification_sent

50a9bb7

set APP_ERROR_XXX_ONCE to False

sonus21 reviewed Oct 20, 2020

View reviewed changes

linting in model

0d12fe8

rename cookie_parse to parse_headers

sonus21 merged commit 20e7c43 into sonus21:master Oct 21, 2020

		@@ -0,0 +1,28 @@
		# Generated by Django 3.1.2 on 2020-10-18 11:17

Uh oh!

Conversation

tboulogne commented Oct 18, 2020

Uh oh!

sonus21 commented Oct 19, 2020

Uh oh!

tboulogne commented Oct 19, 2020 via email

Uh oh!

tboulogne commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tboulogne commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tboulogne commented Oct 19, 2020

Uh oh!

sonus21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tboulogne commented Oct 20, 2020

Uh oh!

sonus21 commented Oct 20, 2020

Uh oh!

tboulogne commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tboulogne commented Oct 20, 2020

Uh oh!

sonus21 commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tboulogne commented Oct 20, 2020 •

edited

Loading