Accumulate Time Lost from comments sent through LOVE #275

Vebop · 2024-08-14T20:29:23Z

Added update_time_loss() to request jira ticket by ID, and updates the existing time_lost value adding what was given in the form to the original value from the ticket.
Called this function from jira_comment() only if 'time_lost' is provided in the request_data.

Updated test_jira_comment() to patch this new function.
Added test_update_time_loss.

pothiers · 2024-08-21T18:12:54Z

manager/api/tests/test_jira.py

+    def test_update_time_loss(self):
+        """Test call to update_time_loss and verify field was updated"""
+        # patch both requests.get, requests.put
+        mock_jira_patcher = patch("requests.get")
+        mock_jira_client = mock_jira_patcher.start()
+        response = requests.Response()
+        response.status_code = 200
+        response.json = lambda: {"customfield_10106": 13.6}
+        mock_jira_client.return_value = response
+
+        mock_jira_patcher = patch("requests.put")
+        mock_jira_client = mock_jira_patcher.start()
+        response = requests.Response()
+        response.status_code = 200
+        mock_jira_client.return_value = response


Replace with patch decorators to simplify. No need to create response twice, me thinks.

@patch("requests.get")
@patch("requests.post")
def test_update_time_loss(self, mock_get, mock_post):
"""Test call to update_time_loss and verify field was updated"""
# patch both requests.get, requests.put
response = requests.Response()
response.status_code = 200
mock_get.return_value = response

response.json = lambda: {"customfield_10106": 13.6} mock_post.return_value = response

I considered that and saw the pattern in this file was to use the decorators for patching environment variables and the implemented way for requests. I wasn't familiar with any best practices that designate one way or the other. However I do like this way so I'll update it.

sebastian-aranda

Good job @Vebop ! This looks good, but I left several comments for the sake of explicity, hope you can take a look 🙏

sebastian-aranda · 2024-08-22T00:13:26Z

manager/manager/utils.py

@@ -505,6 +505,57 @@ def jira_ticket(request_data):
    )


+def update_time_loss(jira_id: int, add_time_loss: float = 0.0) -> Response:
+    """Connect to the Rubin Observatory JIRA Cloud REST API to
+    update a jira ticket's specific field


We should specify here that were are updating the specific time_lost field.

manager/manager/utils.py

sebastian-aranda · 2024-08-22T00:20:54Z

manager/manager/utils.py

+    )
+    response = requests.get(get_url, headers=headers)
+    existent_time_loss = (
+        response.json().get("customfield_10106", 0.0)


This might be too much to ask, as I should have done it before 😅 ... but for the sake of having better maintainable software I'll ask anyways... Could you move this constant: customfield_10106 to a variable (see begining of the utils.py file)? Also, if you could move the other customfield_ constants it would be awesome 🙏.

sebastian-aranda · 2024-08-22T00:23:31Z

manager/manager/utils.py

+            "customfield_10106": float(existent_time_loss + add_time_loss),
+        },
+    }
+    put_url = f"https://{os.environ.get('JIRA_API_HOSTNAME')}/rest/api/latest/issue/{jira_id}/"


I see the difference between get_url and put_url is only a / at the end for the later one. Is this necessary? I'm asking basically if we could use the same url for both purposes.

sebastian-aranda · 2024-08-22T00:24:59Z

manager/manager/utils.py

+    put_url = f"https://{os.environ.get('JIRA_API_HOSTNAME')}/rest/api/latest/issue/{jira_id}/"
+    response = requests.put(put_url, json=jira_payload, headers=headers)
+
+    if response.status_code == 200 or response.status_code == 204:


In which case we would be receiving a status_code = 204?

This is a good question that made me realize the response is only ever 204 for success, not 200. When the update is sent successfully we receive a 204 No Content.

nice, good to know! Just sharing a nice resource here in case you find it usfeul 🤭: https://http.cat/

manager/manager/utils.py

sebastian-aranda · 2024-08-22T00:32:41Z

manager/api/tests/test_jira.py

+    @patch("requests.get")
+    @patch("requests.put")


Not an obligation, but for the sake of standardization could you use the format as on the test_add_comment function (without fixtures)? Again, not an obligation.

Sorry, just saw @pothiers review... One sec, I'm looking at the comments.

ok, so this is one of those things that can be or cannot be jaja. I think is up to the developer, but the thing is (IIRC) this would be the first place on this repo that uses this fixtures format. Again, not an obligation, up to you 😉. But, we should standardize this at some point and add some details to the README.md to point into a standard direction.

sebastian-aranda · 2024-08-22T00:37:19Z

manager/api/tests/test_jira.py

+        mock_timeloss_patcher = patch("manager.utils.update_time_loss")
+        mock_timeloss_client = mock_timeloss_patcher.start()
+        mock_timeloss_client.return_value = response


I left a comment before in regards of returning a 400 response in the case the update_time_loss fails (on the jira_comment function). If you are willing to add that suggestion, that should be tested here (ensuring we get the bad response arranged by the update_time_loss function, in this case mocked up).

sebastian-aranda · 2024-08-22T00:39:48Z

manager/api/tests/test_jira.py

@@ -311,6 +316,27 @@ def test_needed_parameters(self):

        mock_jira_patcher.stop()

+    @patch("requests.get")
+    @patch("requests.put")
+    def test_update_time_loss(self, mock_get, mock_put):


Perhaps you could also test when the update_time_loss fails and returns a 400 status code, ensuring we return the arranged ack (e.g. "Jira time_lost field could not be updated" in case you accept my other suggestion too).

I learned that when we use requests.Response() as our patch in the unit tests, that this is not the same as rest_framework.response.Response (that we set in utils.py). So when we start to mock one of these, we cannot then check what the 'ack' would have been, since we are patching that object.

applied suggestion Co-authored-by: Sebastian Aranda Sanchez <[email protected]>

Co-authored-by: Sebastian Aranda Sanchez <[email protected]>

…equests Response

sebastian-aranda

Looks good! thanks for the great work here @Vebop 💪.

Vebop marked this pull request as ready for review August 21, 2024 00:16

Vebop added 3 commits August 20, 2024 17:30

Implemented function to update time lost attribute in jira ticket

2bad171

Updated test_jira_comment and added test for new function

90ea9e5

Updated Changelog

b200846

Vebop force-pushed the tickets/DM-45776 branch from 86351af to b200846 Compare August 21, 2024 00:33

Vebop requested review from sebastian-aranda and pothiers August 21, 2024 00:33

pothiers approved these changes Aug 21, 2024

View reviewed changes

Implemented PR test patching suggestion

4d00564

sebastian-aranda requested changes Aug 22, 2024

View reviewed changes

Vebop and others added 6 commits August 22, 2024 09:26

Update manager/manager/utils.py

d3dd21f

applied suggestion Co-authored-by: Sebastian Aranda Sanchez <[email protected]>

Update manager/manager/utils.py

eb50b77

Co-authored-by: Sebastian Aranda Sanchez <[email protected]>

Update manager/manager/utils.py

6cd9b4a

Co-authored-by: Sebastian Aranda Sanchez <[email protected]>

Updating comments and using constant for custom fields

907b1c8

handling more comments from pr

ee9bd84

Added test to fail jira comment, Django Response is not the same as r…

4951cf8

…equests Response

Vebop requested a review from sebastian-aranda August 22, 2024 23:29

Mock using same response object from correct framework

00a639d

sebastian-aranda approved these changes Aug 23, 2024

View reviewed changes

Vebop merged commit ee8cac7 into develop Aug 23, 2024
2 checks passed

Vebop deleted the tickets/DM-45776 branch August 23, 2024 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accumulate Time Lost from comments sent through LOVE #275

Accumulate Time Lost from comments sent through LOVE #275

Vebop commented Aug 14, 2024 •

edited

Loading

pothiers Aug 21, 2024

Vebop Aug 21, 2024

sebastian-aranda left a comment •

edited

Loading

sebastian-aranda Aug 22, 2024

sebastian-aranda Aug 22, 2024

sebastian-aranda Aug 22, 2024 •

edited

Loading

sebastian-aranda Aug 22, 2024

Vebop Aug 22, 2024

sebastian-aranda Aug 22, 2024

sebastian-aranda Aug 22, 2024

sebastian-aranda Aug 22, 2024

sebastian-aranda Aug 22, 2024 •

edited

Loading

sebastian-aranda Aug 22, 2024

sebastian-aranda Aug 22, 2024

Vebop Aug 22, 2024

sebastian-aranda left a comment

Accumulate Time Lost from comments sent through LOVE #275

Accumulate Time Lost from comments sent through LOVE #275

Conversation

Vebop commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-aranda left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-aranda Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-aranda Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-aranda left a comment

Choose a reason for hiding this comment

Vebop commented Aug 14, 2024 •

edited

Loading

sebastian-aranda left a comment •

edited

Loading

sebastian-aranda Aug 22, 2024 •

edited

Loading

sebastian-aranda Aug 22, 2024 •

edited

Loading