Feature allow employee to join project by maciejSamerdak · Pull Request #21 · Code-Poets/sheetstorm

maciejSamerdak · 2019-01-28T13:10:51Z

Resolves #42
Resolves #105

kbeker

I'm tired of reviewing all this PR's but this is the last one :D Please apply my changes

kbeker · 2019-02-06T13:15:06Z

+    def __init__(self, queryset, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        assert isinstance(queryset, QuerySet)
+        # assert isinstance(queryset.model, Project)      No exception message supplied


Delete this comment

kbeker · 2019-02-06T13:15:49Z

+        project.members.add(self.request.user)
+        project.full_clean()
+        project.save()
+        project.refresh_from_db()


Is it necessary here? I think it should work without refreshing database.

kbeker · 2019-02-06T13:24:06Z

        reports_serializer = ReportSerializer(data=request.data, context={'request': request})
-        reports_dict = query_as_dict(self.get_queryset())
+
+        if 'join' in request.POST:


This could be changed to some kind of Enum and reused in place where it is assigned to request. Thanks to this will not stop working if it will be changed in another place

You mean the 'join' string? It's an id from HTML template, so I'd say it's name is rather irrelevant, but I could supply some sort of constant for it. The question is whether it's worth the extra effort?

Yea, you're right. It won't be necessary :)

kbeker · 2019-02-06T13:24:23Z

+                'project_form': self.project_form,
+            })
+
        if not reports_serializer.is_valid():


Also please change it into elif

What are the benefits of it?

kbeker · 2019-02-06T13:26:09Z

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


This should be deleted in previous PR. What save() method returns if something goes wrong should be investigated.

kbeker · 2019-02-06T13:26:29Z

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


Move this function to .js file

kbeker · 2019-02-06T13:28:11Z

+        queryset_length = 10
+        for i in range(queryset_length):
+            self.project = Project(
+                name="Test Project %d" % i,


Please change it into f-string

rwrzesien · 2019-02-28T02:03:30Z

@maciejSamerdak It would be good to create an issue for what you are adding with this pull request, or link to an existing one.

kbeker

Generally OK. Please squash fixups and apply one thing which I mentioned in comment

kbeker · 2019-03-28T16:48:00Z

+        response = ReportDetail.as_view()(request, pk=self.report.pk)
+        self.report.refresh_from_db()
+        self.assertEqual(response.status_code, 200)
+        self.assertIsNotNone(response.data['errors'])


Please change this assert that you check exactly what error is in this dict

In order to get to the ErrorDetail object, I have to do this:
response.data['errors']['project'][0]

Then I can either get to the code
response.data['errors']['project'][0].code = 'does_not_exist'
or to the (very specific) message
str(response.data['errors']['project'][0]) = 'Object with name=Other Project does not exist.'

Which one should I use? Or should just narrow it down to checking if field specific errors exist? self.assertIsNotNone(response.data['errors']['project'])

@maciejSamerdak I would prefer to check response.data['errors']['project'][0].code = 'does_not_exist' but I know that to change it everywhere is very big effort, so please just apply this to this PR.

kbeker · 2019-03-28T16:50:09Z

        reports_serializer = ReportSerializer(data=request.data, context={'request': request})
-        reports_dict = query_as_dict(self.get_queryset())
+
+        if 'join' in request.POST:


Yea, you're right. It won't be necessary :)

Adds form with singular `ChoiceField` which is supposed to contain a list of projects the user can join to.

Due to increased complexity of its' functionality, the class' structure has been heavily modified, introducing helper methods and additional fields. Most important of all, an `initial` method, which contains actions to be performed before each HTTP call, has been overridden to include initiation of aforementioned fields.

Added javascript and HTML code for pop-up functionality

Limits projects displayed in project field in report detail view to only those which author is a member of.

Adds tests for project join form and extends tests in custom views, regarding applied changes to code.

kbeker

Ready to merge

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

maciejSamerdak force-pushed the feature-accurate-hour-display branch from 1b8b92a to 4bf6d9e Compare February 5, 2019 13:05

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from 5c8805c to 4c22d16 Compare February 5, 2019 13:07

kbeker reviewed Feb 6, 2019

View reviewed changes

Karrp assigned maciejSamerdak Feb 25, 2019

maciejSamerdak force-pushed the feature-accurate-hour-display branch from 802317d to 620bd5b Compare February 25, 2019 15:35

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from ba63f8b to b899baa Compare February 25, 2019 15:37

maciejSamerdak force-pushed the feature-accurate-hour-display branch from 942cd4b to 3386a7d Compare February 26, 2019 13:35

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from 2823264 to 328f7b0 Compare February 26, 2019 13:35

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

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from d32b90a to c0e9652 Compare March 20, 2019 16:21

maciejSamerdak force-pushed the feature-accurate-hour-display branch from 3386a7d to 78535dd Compare March 20, 2019 16:22

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from 93f13c4 to 805e3a5 Compare March 22, 2019 13:28

maciejSamerdak force-pushed the feature-accurate-hour-display branch from 78535dd to 693846b Compare March 22, 2019 14:33

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from 805e3a5 to d52c409 Compare March 22, 2019 14:34

maciejSamerdak force-pushed the feature-accurate-hour-display branch from 693846b to 1af60a5 Compare March 28, 2019 14:58

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from f2ef2ef to 2470564 Compare March 28, 2019 14:59

maciejSamerdak changed the base branch from feature-accurate-hour-display to master March 28, 2019 15:38

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from 2470564 to 239157f Compare March 28, 2019 16:36

kbeker previously approved these changes Mar 28, 2019

View reviewed changes

maciejSamerdak dismissed kbeker’s stale review via ba8ccc0 April 1, 2019 11:44

maciejSamerdak force-pushed the feature-allow-employee-to-join-project branch from ba8ccc0 to 083c20c Compare April 1, 2019 11:49

maciejSamerdak added 4 commits April 1, 2019 17:17

Add form for project joining feature

2fd0dd7

Adds form with singular `ChoiceField` which is supposed to contain a list of projects the user can join to.

Add join feature related GUI text for report list view

8a51fff

Add script for join form's popup in report list

5a00f3d

maciejSamerdak added 3 commits April 1, 2019 17:17

Add feature to template in form of a pop-up window

c699eb4

Added javascript and HTML code for pop-up functionality

Limit project choice in report detail view

3ef31a9

Limits projects displayed in project field in report detail view to only those which author is a member of.

Add unit tests for join feature

dbc9487

Adds tests for project join form and extends tests in custom views, regarding applied changes to code.

kbeker force-pushed the feature-allow-employee-to-join-project branch from 083c20c to dbc9487 Compare April 1, 2019 15:18

kbeker approved these changes Apr 1, 2019

View reviewed changes

kbeker merged commit dbc9487 into master Apr 1, 2019

kbeker deleted the feature-allow-employee-to-join-project branch April 1, 2019 15:18

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!

rwrzesien commented Feb 28, 2019

Uh oh!

kbeker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

maciejSamerdak Mar 28, 2019 •

edited

Loading