Skip to content
This repository has been archived by the owner on May 26, 2021. It is now read-only.

#17 display header in column #46

Merged
merged 24 commits into from
Dec 13, 2020

Conversation

PetrS12
Copy link
Contributor

@PetrS12 PetrS12 commented Oct 17, 2020

closes #17

@codecov
Copy link

codecov bot commented Oct 17, 2020

Codecov Report

Merging #46 (18044bf) into develop (d780eec) will increase coverage by 0.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #46      +/-   ##
===========================================
+ Coverage    95.96%   96.32%   +0.35%     
===========================================
  Files            5        5              
  Lines          124      136      +12     
  Branches         5        6       +1     
===========================================
+ Hits           119      131      +12     
  Misses           4        4              
  Partials         1        1              
Impacted Files Coverage Δ
http_stubs/templatetags/stub_tags.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d780eec...18044bf. Read the comment docs.

@@ -16,7 +16,7 @@
{% else %}
{{ field.label_tag }}
{% if field.is_readonly %}
<div class="readonly">{{ field.contents }}</div>
{% if field.field.name == 'headers' %}<ul class="readonly">{% for header in field.contents|str_to_list:"', " %}<li>{{ header|cut:"'" }}</li>{% endfor %}</ul>{% else %}<div class="readonly">{{ field.contents }}</div>{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This approach looks extremely clumsy. With every change we just lay over one more layer of unreadable template code.

I'd suggest trying something else.

For the forms in general, django-crispy-forms is excellent.

And just for this case, for example, we may try using a separate field type or something.

But just adding ad-hoc conditions for every field will soon make the source harder to understand and maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, django-crispy-forms aren't not very good friends with simpleui, but if I used crispy I would make a template for this field.
I made custom change templates for every model. And set the attribute change_form_template in admin model.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, unfortunately, I never worked with simpleui, and it's a long time since I've last worked with Django forms.
I need some time to experiment myself with Django built-in functionality and/or crispy.
Crispy forms allow to use any custom template for any field as far as I remember, although this might also look a bit clumsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to leave it as it is.

@lowitea lowitea self-requested a review October 19, 2020 19:11
@@ -16,3 +18,15 @@ def get_absolute_url_tag(context: Dict, url: Url) -> Url:
:returns: absolute url
"""
return context.get('request').build_absolute_uri(url)


@register.filter(name='headers_to_list')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: if we call function headers_to_list, then we won't have to specify the name as the register argument. There's no need to give distinct names to the function and the filter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this will break if some header contains separator. I think this is highly probable for the text headers, maybe using json module for proper loading of the headers would be more robust?..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

@register.filter(name='headers_to_list')
@stringfilter
def get_headers_list(headers: AnyStr, separator: AnyStr) -> List:
"""Filter that creates a list from a string of headers(dict).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd start from Python str.split description because it is more clear. E.g.:
Return a list of the lines in the string representation of the request headers, using sep as the delimiter string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

def get_headers_list(headers: AnyStr, separator: AnyStr) -> List:
"""Filter that creates a list from a string of headers(dict).

:param headers: string of headers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
:param headers: string of headers
:param headers: string representation of the request headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

"""Filter that creates a list from a string of headers(dict).

:param headers: string of headers
:param separator: separator to separate string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
:param separator: separator to separate string
:param separator: delimiter string

(also from str.split docs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this part of code.


:param headers: string of headers
:param separator: separator to separate string
:returns: list of headers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
:returns: list of headers
:returns: list of individual headers string representation

(nit: unsure, but list of headers is too vague in my opinion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

@@ -2,4 +2,4 @@

wait-for-it "${PARROT_DB_HOST:-parrot-database}":5432 -s -t 180 \
&& python /app/src/manage.py migrate --noinput \
&& python /app/src/manage.py runserver 0.0.0.0:8042
&& python /app/src/manage.py runserver 127.0.0.1:8042
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it should be in a separate PR or a debug change that made its way into production code. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

{% block writable_field %}
{{ field.field }}
{# added request url for path field(issue #35) #}
{% if field.field.name == 'path' %}<div class="readonly"><div id="full-path">{% absolute field.field.value %}</div><div id="path-copied-alert">Скопировано!</div></div>{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cyrillic characters still remain. Also, HTML doesn't impose the requirement to have everything in one extremely long line.
HTML can be broken at almost any boundary, this can greatly enhance readability of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

@lowitea lowitea added this to the 1.0.0 milestone Dec 7, 2020
@lowitea
Copy link
Contributor

lowitea commented Dec 12, 2020

@PetrS12 could you please rebase this branch

@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lowitea lowitea merged commit e5d9672 into Uma-Tech:develop Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display the list of headings in a column on the log page
3 participants