Feature employees interface by maciejSamerdak · Pull Request #18 · Code-Poets/sheetstorm

maciejSamerdak · 2019-01-28T13:06:45Z

Resolves #39, resolves #48

kbeker

Few parts of this code needs improvement

.

kbeker · 2019-02-05T14:36:38Z

+        $('#dialog').dialog('open');
+    });
+});
+</script>


Please move this javascript function to .js file

kbeker · 2019-02-05T14:51:44Z

+            </tr>
+            {% for date, reports in reports_dict.items %}
+            <tr>
+                <td style="width:128px" rowspan={{ reports|length }}>


Please move all inline styling to .css file

kbeker · 2019-02-06T09:47:55Z

+
+<h1>{{ UI_text.PAGE_TITLE.value }}{{ report.project }} ({{ report.date }})</h1>
+</br>
+<div class="modal-dialog" style="margin-bottom:0">


If you have any styling please move it to .css file

kbeker · 2019-02-06T10:08:21Z

+    def test_queryset_as_dict_should_return_dictionary_where_keys_are_dates_and_values_are_lists_of_reports(self):
+        queryset = Report.objects.all()
+        dictionary = query_as_dict(queryset)
+        self.assertEqual(len(dictionary.items()), 2)


You don't have to convert it into list. len() function works also with dicts :)

kbeker · 2019-02-06T10:36:23Z

+        self.assertTrue(isinstance(list(dictionary.keys())[1], datetime.date))
+        self.assertTrue(isinstance(list(dictionary.values())[0][0], Report))
+        self.assertTrue(isinstance(list(dictionary.values())[0][1], Report))
+        self.assertTrue(isinstance(list(dictionary.values())[1][0], Report))


You don't have to use self.assertTrue to all of them. It is enough to have only self.isinstance(). Also you don't have to use list converting as well, as I wrote above.

List conversion was required for indexing, because neither dictionary.values() nor dictionary.keys() return an indexable type.

kbeker · 2019-02-06T12:07:24Z

+        key = record.date
+        dictionary.setdefault(key, [])
+        dictionary[key].append(record)
+    return dictionary


I saw this function in @MartynaAnnaGottschling PR. It should be in utils and be reusable for more than one app.

I can tell you right now, that it will change it's functionality later on to be more specific. It's used to return specific set of data, designed for rendering lists of reports in requested format. Granted such method might be potentially useful in other places, but in reality we store all report related views in employees app (because that's were report model and serializer are stored in the first place).

And @MartynaAnnaGottschling won't be using it anymore, because it was useless for her anyway. It also actually performed different action than mine.

Do you still want me to move it to utils? Like I said, it's a specific tool meant to be used with employees app only.

kbeker · 2019-02-06T12:26:23Z

+            'reports_dict': reports_dict,
+            'UI_text': ReportListStrings,
+        }, status=201)
+        # QUESTION: Should I use some sort of database verification prior to returning Response with status 201?


I think that we will need to think about it how to do database verification. If any error occurs will save() method raise exception? Please investigate it.

So after my thorough investigation, it turns out that Django in fact throws an AssertionError if serializer.save() is called without serializer.is_valid() preceding it. Thus it forces you to perform validation check on serializer before saving it's data in database.
So I sabotaged the serializer to have it pass incomplete data to model. In such case an IntegrityError is thrown if model receives bad data.

kbeker · 2019-02-06T12:27:23Z

+            serializer = ReportSerializer(
+                report, data=request.data,
+                context={'request': request}
+            )


Please change it into:

serializer = ReportSerializer( report, data=request.data, context={'request': request} )

kbeker · 2019-02-06T12:27:43Z

             <td style="padding:0 15px 0 15px;"><a href="{% url 'custom-projects-list' %}">{% trans 'Projects list' %}</a></td>
             {% endif %}
+             <td style="padding:0 15px 0 15px;"><a href="{% url 'custom-report-list' %}">{% trans 'Reports' %}</a></td>
             <td style="padding:0 15px 0 15px;"><a href="{% url 'logout' %}">{% trans 'Logout' %}</a></td>


Please move style to .css file

kbeker · 2019-02-06T12:28:12Z

@@ -0,0 +1,3 @@
+def notCallable(cls):
+    cls.do_not_call_in_templates = True
+    return cls


Is this decorator necessary?

Yes, it is absolutely necessary in order to make enums accessible in templates. Otherwise no value is displayed after render.
I forgot what was exactly the issue, but it has something to do with calls in Django.

rwrzesien · 2019-02-28T02:11:27Z

@maciejSamerdak After some digging I see this is the starting pull request for the three following up :) But why it is being compared to feature-managers-app branch for which there is no pull request currently?

kbeker

Just one thing to clarify and ready to merge.

After using this decorator, enum values can be easily accessed from templates.

Also pushes existing DRF views to '*/api/' path

kbeker

Ready to merge

maciejSamerdak requested review from Karrp, MartynaAnnaGottschling, TheCM and kbeker January 28, 2019 13:06

MartynaAnnaGottschling force-pushed the feature-managers-app branch from 0ad3c33 to c45eb4d Compare February 4, 2019 10:12

kbeker force-pushed the feature-employees-interface branch from d572781 to 9abc042 Compare February 5, 2019 09:54

MartynaAnnaGottschling force-pushed the feature-managers-app branch from c45eb4d to f0292c2 Compare February 5, 2019 12:49

maciejSamerdak force-pushed the feature-employees-interface branch from 9abc042 to 3448416 Compare February 5, 2019 12:59

kbeker suggested changes Feb 6, 2019

View reviewed changes

MartynaAnnaGottschling force-pushed the feature-managers-app branch 3 times, most recently from b172f97 to d6924dd Compare February 21, 2019 10:13

Karrp assigned maciejSamerdak Feb 25, 2019

MartynaAnnaGottschling force-pushed the feature-managers-app branch from d6924dd to 18687db Compare February 25, 2019 10:23

maciejSamerdak force-pushed the feature-employees-interface branch 2 times, most recently from 68c67fd to a38ed81 Compare February 26, 2019 13:33

MartynaAnnaGottschling force-pushed the feature-managers-app branch from 18687db to d5f74fb Compare February 27, 2019 14:46

maciejSamerdak added this to the tag 0.1.0 milestone Mar 4, 2019

MartynaAnnaGottschling force-pushed the feature-managers-app branch 2 times, most recently from 0a10086 to 9e1a6e3 Compare March 8, 2019 13:41

maciejSamerdak force-pushed the feature-employees-interface branch from 79e6049 to de9086a Compare March 22, 2019 14:33

kbeker previously approved these changes Mar 28, 2019

View reviewed changes

Comment thread employees/templates/employees/report_list.html

maciejSamerdak changed the base branch from feature-managers-app to master March 28, 2019 13:36

maciejSamerdak added 6 commits March 28, 2019 15:09

Add custom decorator for loading enums in templates

418cd33

After using this decorator, enum values can be easily accessed from templates.

Add custom views' UI text in string constants

4a571f1

Add style.css file in employees app

b010b1c

Add template for custom Report list view

dc485b9

Add popup script in employees app

9889360

Add template for custom Report detail view

792ec4a

maciejSamerdak added 4 commits March 28, 2019 15:09

Add custom views for Report model

ae75e30

Link custom views in url config

ba56855

Also pushes existing DRF views to '*/api/' path

Add unit tests for custom Report views

f028d64

Link employees app in base template

b3ac00d

maciejSamerdak dismissed kbeker’s stale review via b3ac00d March 28, 2019 14:10

maciejSamerdak force-pushed the feature-employees-interface branch from de9086a to b3ac00d Compare March 28, 2019 14:10

kbeker approved these changes Mar 28, 2019

View reviewed changes

kbeker merged commit b3ac00d into master Mar 28, 2019

kbeker deleted the feature-employees-interface branch March 28, 2019 14:48

Conversation

maciejSamerdak commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbeker 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!

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!

rwrzesien commented Feb 28, 2019

Uh oh!

kbeker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbeker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maciejSamerdak commented Jan 28, 2019 •

edited

Loading