Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accumulate Time Lost from comments sent through LOVE #275

Merged
merged 11 commits into from
Aug 23, 2024
5 changes: 5 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
Version History
===============

v7.0.2
------

* Add accumulation of time lost in Jira comments

v7.0.1
------

Expand Down
32 changes: 31 additions & 1 deletion manager/api/tests/test_jira.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@
import requests
from django.test import TestCase, override_settings

from manager.utils import handle_jira_payload, jira_comment, jira_ticket
from manager.utils import (
handle_jira_payload,
jira_comment,
jira_ticket,
update_time_loss,
)

OLE_JIRA_OBS_COMPONENTS_FIELDS = [
"AuxTel",
Expand Down Expand Up @@ -311,6 +316,27 @@ def test_needed_parameters(self):

mock_jira_patcher.stop()

@patch("requests.get")
@patch("requests.put")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sebastian-aranda sebastian-aranda Aug 22, 2024

Choose a reason for hiding this comment

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

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.

def test_update_time_loss(self, mock_get, mock_put):
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"""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_put.return_value = response

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

# call update time lost
jira_response = update_time_loss(1, 3.4)
assert jira_response.status_code == 200
assert jira_response.data["ack"] == "Jira field updated"

jira_response = update_time_loss(93827, 1.23)
assert jira_response.status_code == 200
assert jira_response.data["ack"] == "Jira field updated"

def test_add_comment(self):
"""Test call to jira_comment function with all needed parameters"""
mock_jira_patcher = patch("requests.post")
Expand All @@ -319,6 +345,10 @@ def test_add_comment(self):
response.status_code = 201
mock_jira_client.return_value = response

mock_timeloss_patcher = patch("manager.utils.update_time_loss")
mock_timeloss_client = mock_timeloss_patcher.start()
mock_timeloss_client.return_value = response
Copy link
Contributor

Choose a reason for hiding this comment

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

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).


jira_response = jira_comment(self.jira_request_exposure_full_jira_comment.data)
assert jira_response.status_code == 200
assert jira_response.data["ack"] == "Jira comment created"
Expand Down
57 changes: 57 additions & 0 deletions manager/manager/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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


Params
------
jira_id: int
Jira ID

Returns
-------
Response
define response here
Vebop marked this conversation as resolved.
Show resolved Hide resolved
"""
headers = {
"Authorization": f"Basic {os.environ.get('JIRA_API_TOKEN')}",
"content-type": "application/json",
}
get_url = (
f"https://{os.environ.get('JIRA_API_HOSTNAME')}/rest/api/latest/issue/{jira_id}"
)
response = requests.get(get_url, headers=headers)
existent_time_loss = (
response.json().get("customfield_10106", 0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙏.

if response.status_code == 200
else 0.0
)
jira_payload = {
"fields": {
"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}/"
Copy link
Contributor

@sebastian-aranda sebastian-aranda Aug 22, 2024

Choose a reason for hiding this comment

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

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.

response = requests.put(put_url, json=jira_payload, headers=headers)

if response.status_code == 200 or response.status_code == 204:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return Response(
{
"ack": "Jira field updated",
"url": f"https://{os.environ.get('JIRA_API_HOSTNAME')}/browse/{jira_id}",
},
status=200,
)
return Response(
{
"ack": "Jira field could not be updated",
Vebop marked this conversation as resolved.
Show resolved Hide resolved
},
status=400,
)


def jira_comment(request_data):
"""Connect to the Rubin Observatory JIRA Cloud REST API to
make a comment on a previously created ticket.
Expand Down Expand Up @@ -547,6 +598,12 @@ def jira_comment(request_data):
}
url = f"https://{os.environ.get('JIRA_API_HOSTNAME')}/rest/api/latest/issue/{jira_id}/comment"
response = requests.post(url, json=jira_payload, headers=headers)

if "time_lost" in request_data:
update_time_loss(
jira_id=jira_id, add_time_loss=request_data.get("time_lost", 0.0)
)
Vebop marked this conversation as resolved.
Show resolved Hide resolved

if response.status_code == 201:
return Response(
{
Expand Down
Loading