-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Update and Add OpenProject integration #34028
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
Update and Add OpenProject integration #34028
Conversation
Hello @zulip/server-integrations members, this pull request was labeled with the "area: integrations" label, so you may want to check it out! |
This PR fixes the issues pointed out in #29944. Write a fresh
Other changes:
Do review this PR and let me know if any changes have to be made. |
Thank you for the updates. They are good improvements. When I mentioned writing a "fresh" view.py, I didn't want the next contributor taking any inspiration from the previous view.py. Can you please share the info you gathered or relevant links regarding the default values of parameters like "work package type", "action status", etc.? Use Reminder: https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html I'm wondering if we could drop the "successfully" word, given that it's part of all of the message templates. You also need to ensure coverage - see the failing checks. Ah, I'm seeing that you've noticed some of these yourself. |
cd0d994
to
eb29f36
Compare
0f68aeb
to
bcf9d6a
Compare
@Niloth-p are there any pending issues or is this pr ready to be merged? |
It's not ready to be merged, there are several rounds of review still pending. |
@Niloth-p Any updates on what needs to be added or fixed for this integration? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version definitely looks much improved. It's so much more easy to read now. Thank you for working on this!
I've left code suggestions for how the code can be optimized. Please verify the payload fields, and share the info and/or links that you find. The topic value needs to be re-considered.
Use git rebase
not git merge
. It's fine in this case, since we'll be squashing anyways. But, please read https://zulip.readthedocs.io/en/latest/contributing/continuing-unfinished-work.html
zerver/webhooks/openproject/view.py
Outdated
from zerver.lib.webhooks.common import check_send_webhook_message | ||
from zerver.models import UserProfile | ||
|
||
EVENTS: dict[str, str] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace lines 12-43. That's way too verbose, and not optimized.
EVENTS: dict[str, str] = { | |
ALL_EVENT_TYPES: list[str] = [ | |
"project:created", | |
"project:updated", | |
"work_package:created", | |
"work_package:updated", | |
"time_entry:created", | |
"attachment:created", | |
] | |
WORKPACKAGE_TYPES: list[str] = ["Task", "Milestone", "Phase"] | |
PROJECT_MESSAGE_TEMPLATE = "Project **{name}** was {action}." | |
WORK_PACKAGE_MESSAGE_TEMPLATE = "**{type}** work package **{subject}** was {action}." | |
ATTATCHMENT_MESSAGE_TEMPLATE = "File **{filename}** was uploaded." | |
TIME_ENTRY_MESSAGE_TEMPLATE = "A time entry of **{hours}** was {action} for project **{project}**." |
- There's no reason to use an events dict.
- Do not use commas at the end of a list or dict, they force un-wrap it.
- Use normal quotes, not triple quotes for the templates. Unless you believe that there's a possibility for any of them to have quote characters inside the template.
- I've edited a couple templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a much better approach and has been added.
zerver/webhooks/openproject/view.py
Outdated
payload: JsonBodyPayload[WildValue], | ||
) -> HttpResponse: | ||
try: | ||
action: str = payload["action"].tame(check_string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace lines 55-59 with
action: str = payload["action"].tame(check_string) | |
event_type: str = payload["action"].tame(check_string) | |
item, action = event_type.split(":") | |
action_data: WildValue = payload[item] |
- Use split to get the values. Using the dict for that is just redundancy.
- Rename the variables like above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a better approach and has been added.
zerver/webhooks/openproject/view.py
Outdated
action_status: str = action.split(":")[1] | ||
|
||
action_data: WildValue = payload[EVENTS[action]] | ||
topic: str = EVENTS[action].replace("_", " ").title() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense.
Why would we want to use "work package", "attachment", or "project" as Zulip topics?
Think from the POV of the user. How would you want your notifications to be grouped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the perspective of the user and looking at other integrations, I removed this and set the topic according to the openproject project name. Since the project name is available differently in all payloads the topic is dynamically defined in each event.
zerver/webhooks/openproject/view.py
Outdated
|
||
check_send_webhook_message(request, user_profile, topic, message) | ||
return json_success(request) | ||
except ValidationError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how we do the validation.
You must have noticed that we don't have the entire functions inside a try-except block for all the webhook integrations.
You need to check the type of the fields returned in the payload. This is what I was asking when I asked for the "default values" in the last review round. Not the types of work packages or such, but the default value, for example, could it be an empty string, undefined or null? Are the fields mandatory or optional?
For example, you're parsing project like this - project=action_data["_embedded"]["project"]["name"].tame(check_string)
, but did you verify that action_data will always have those nested fields? I do think that it's likely that it will, but one needs to still verify that, and check for any edge cases.
We need to verify the type of each payload parameter that we parse from their API docs or their YAML, if the docs don't have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This try - except has been removed. There is very minimal documentation available on specific json data. After manual testing I believe these keys will always exist and no validation error would occur. Also since we are showing very minimal surface level notifications this should be good to go.
zerver/webhooks/openproject/view.py
Outdated
action_status=action_status, | ||
) | ||
case "attachment": | ||
message = ATTATCHMENT_MESSAGE_TEMPLATE.format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
bcf9d6a
to
09b4e0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes as suggested, and quickly!
I've left some suggestions on the doc that you can absorb.
Great, we're almost there. Let's do another round on the message templates, now that the topic is set as the project name.
- We don't have to include the project name in the time-entry message template, as it's already part of the topic.
- Would it make sense to add the work package name to the attachment and time entry message templates?
- We don't seem to have the user being mentioned in any of the message templates? I think OpenProject is actually designed for teams, right, so shouldn't that be included to know who did what? Can you see for which of the templates it makes sense to include the user name? Because we don't want to sound too repetitive either.
- Did you check if OpenProject includes (hashed) links to the file attachments in the payload? If they do, we could format the file attachments like this.
- It seems like the time entry includes a link too? Can you check that? We should be including all such links.
- When a project is created, we'd also want to show fields like the description, and mention parent project names, if they're nested.
Using these as inspiration, can you please take a look at all the fixtures and their fields once to identify details that we'd want to include in our messages?
And if they do, format them nicely into a readable sentence (as opposed to key-value pairs).
zerver/webhooks/openproject/doc.md
Outdated
@@ -0,0 +1,33 @@ | |||
# Zulip Open Project integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Zulip Open Project integration | |
# Zulip OpenProject integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
zerver/webhooks/openproject/doc.md
Outdated
@@ -0,0 +1,33 @@ | |||
# Zulip Open Project integration | |||
|
|||
Get Zulip notifications for new events on your **Open Project** page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get Zulip notifications for new events on your **Open Project** page. | |
Get Zulip notifications for your **OpenProject** activities! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
zerver/webhooks/openproject/doc.md
Outdated
|
||
1. {!generate-webhook-url-basic.md!} | ||
|
||
1. Sign in to OpenProject and create your organization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lines 11-19
1. Sign in to OpenProject and create your organization. | |
1. From your OpenProject organization, click on your user profile icon, | |
choose **Administration**, select **API and Webhooks**, and navigate to | |
the **Webhooks** tab from the left panel. | |
1. Click on **+ Webhook**. Enter a name of your choice for the webhook, | |
set `Payload URL` to the URL generated above, and ensure the webhook is | |
enabled. | |
1. Select the events and projects you want to receive notifications for, and | |
click **Create**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
zerver/webhooks/openproject/doc.md
Outdated
|
||
{!webhooks-url-specification.md!} | ||
|
||
[1]: https://www.openproject.org/docs/system-admin-guide/api-and-webhooks/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[1]: https://www.openproject.org/docs/system-admin-guide/api-and-webhooks/ | |
[1]: https://www.openproject.org/docs/system-admin-guide/api-and-webhooks/#webhooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
09b4e0b
to
a60ca43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for checking all of them, and adding all those new fixtures and tests. That's quite helpful.
Could you please rename the fixtures like this - "project_created__with_parent"
and so on, in the form of "{item}_{action}__{suffix}"
. That's the convention we typically use.
Ah, yes, this table's constraints column is what I was asking for previously. Kudos on locating it! And thanks for sharing the screenshots. Could you share the URL as well?
zerver/webhooks/openproject/view.py
Outdated
TIME_ENTRY_MESSAGE_TEMPLATE = "A time entry of **{hours}** was {action} by **{user}**." | ||
|
||
TIME_ENTRY_WITH_WORKPACKAGE_MESSAGE_TEMPLATE = ( | ||
"A time entry of **{hours}** was {action} by **{user}** for workpackage **{workpackage}**." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
"A time entry of **{hours}** was {action} by **{user}** for workpackage **{workpackage}**." | |
"**{user}** {action} a time entry of **{hours}** for **{workpackage}**." |
I've removed the "workproject" because it seems the time entries can only be for workprojects or projects, so it's implicitly known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is better.
zerver/webhooks/openproject/view.py
Outdated
user=action_data["_embedded"]["user"]["name"].tame(check_string), | ||
) | ||
) | ||
topic = action_data["_embedded"]["project"]["name"].tame(check_string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting that the image you linked says that the project could be different from the workpackage's project, if the workpackage is moved. I think we should send the notifications to the workpackage's project then, if it's mentioned. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no project linked in the workpackage object in the time entry payload. The only project we have is the one mentioned in the image. To fetch the workpackages project we would have to call other apis which will complicate things. We can let this be or make the topic name in case of time entries be workpackage names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the work package does not have an associated project name in the payload that we have, or in all payloads? Because the doc sounded like it would be included at least if the work package was moved to another project?
Oh, we don't want to make the topic the work package's name. The project name is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any sample payload in the docs that includes a work package project when fetching a time entry. This scenario might need to be replicated manually but my OpenProject trial has expired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe when there's no workpackage, we add "for this project" to the message? Hmmm... I guess if they've set the topic in the webhook URL, then that would be misleading. Their docs do say that the project will be not null for a time entry: https://www.openproject.org/docs/api/endpoints/time-entries/#linked-properties.
I think that if there's no work package, then adding the project name to the message makes sense ...
Current:
Nirved Mishra logged 1 hour.
Revised:
Nirved Mishra logged 1 hour on Project1.
That way if the topic is set by the bot's URL, there's still information about where the time entry was logged.
zerver/webhooks/openproject/view.py
Outdated
message = ( | ||
TIME_ENTRY_WITH_WORKPACKAGE_MESSAGE_TEMPLATE.format( | ||
hours=action_data["hours"].tame(check_string).split("T")[1], | ||
project=action_data["_embedded"]["project"]["name"].tame(check_string), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to no longer be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, project has been removed.
zerver/webhooks/openproject/view.py
Outdated
) | ||
|
||
ATTACHMENT_MESSAGE_TEMPLATE = ( | ||
"File **{filename}** was uploaded by **{author}** in {container_type} **{container_name}**." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
"File **{filename}** was uploaded by **{author}** in {container_type} **{container_name}**." | |
"**{author}** uploaded **{filename}** in {container_type} **{container_name}**." |
I assume that you're sure that the container
is not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is much better.
zerver/webhooks/openproject/view.py
Outdated
PROJECT_MESSAGE_TEMPLATE = "Project **{name}** was {action}." | ||
|
||
PROJECT_WITH_PARENT_MESSAGE_TEMPLATE = ( | ||
"Project **{name}** was {action} in project **{parent_project}**." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to show the parent project name everytime a project is updated? I think it's fine just to mention it when we're creating the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed parent project for updated event.
zerver/webhooks/openproject/view.py
Outdated
|
||
PROJECT_MESSAGE_TEMPLATE = "Project **{name}** was {action}." | ||
|
||
PROJECT_WITH_PARENT_MESSAGE_TEMPLATE = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Instead of having a separate
PROJECT_WITH_PARENT_MESSAGE_TEMPLATE
, we can just pass in an extra parameterparent
toPROJECT_MESSAGE_TEMPLATE
with the value" in project **{parent_project}**."
or""
if no parent. - Do the same as above for
TIME_ENTRY_WITH_WORKPACKAGE_MESSAGE_TEMPLATE
, overload it onto theTIME_ENTRY_MESSAGE_TEMPLATE
.
This would significantly simplify the view.py code logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this will make the code cleaner. I have reverted to older template names and updated conditionals.
a0f43c6
to
cd039f4
Compare
Thanks a lot for your review! I have made necessary changes to the the template sentences and renamed fixtures to follow proper convention. Also cleaned the conditionals and stuck to single templates for categories. Other relevant information from the docs:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates!
Apart from some very minor comments, this LGTM.
You can go ahead and squash the commits. Remember to add co-author credits for the previous author, and it would be nice if you could add me too.
zerver/webhooks/openproject/view.py
Outdated
case "project": | ||
parent_project: str = ( | ||
f" in project **{action_data['_embedded']['parent']['name'].tame(check_string)}**" | ||
if "_embedded" in action_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this parent project computation can be simplified by using .get() instead of square brackets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is better and has been added.
zerver/webhooks/openproject/view.py
Outdated
user=action_data["_embedded"]["user"]["name"].tame(check_string), | ||
) | ||
) | ||
topic = action_data["_embedded"]["project"]["name"].tame(check_string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the work package does not have an associated project name in the payload that we have, or in all payloads? Because the doc sounded like it would be included at least if the work package was moved to another project?
Oh, we don't want to make the topic the work package's name. The project name is fine.
zerver/webhooks/openproject/tests.py
Outdated
"attachment_created", | ||
expected_topic, | ||
expected_message, | ||
content_type="application/json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on mobile and I can't check right now, but isn't the default value of content_type json for check_webhook? If so, let's remove this line and the trailing comma from all these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, content_type="application/json"
was the default value and has been removed.
4c236c1
to
5b25b74
Compare
Thanks again for your help with this issue! I've added the co-author credits and squashed the commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this integration!
Once you're done with this update, add the "maintainer review" label using zulipbot, and tag Lauryn Menard requesting a review.
5b25b74
to
20c94d4
Compare
@zulipbot add "maintainer review" |
@laurynmm could you please review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theofficialvedantjoshi - Thanks for working on this integration! I had some comments about the Zulip message formats for the different events that we're handling.
Let me know if you have any questions about my feedback comments below!
zerver/webhooks/openproject/view.py
Outdated
"attachment:created", | ||
] | ||
|
||
WORKPACKAGE_TYPES: list[str] = ["Task", "Milestone", "Phase"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this isn't used. I assume because these types are can be created and deleted and therefore we can't assume what these strings are for all OpenProject integrations: https://www.openproject.org/docs/system-admin-guide/manage-work-packages/work-package-types/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was overlooked and has been removed.
zerver/webhooks/openproject/view.py
Outdated
.tame(check_string) | ||
) | ||
if parent_project and action == "created": | ||
parent_project_message = f" in project **{parent_project}**" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might revise this to " as a sub-project of **{parent_project}**"
since that's a bit clearer in the hierarchy that's being created in the projects here ... https://www.openproject.org/docs/user-guide/projects/#project-structure.
Current:
"Project AI Backend was created in project Demo project."
Revised:
Project AI Backend was created as a sub-project of Demo project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the changed template is clear and has been added.
zerver/webhooks/openproject/view.py
Outdated
PROJECT_MESSAGE_TEMPLATE = "Project **{name}** was {action}{parent_project_message}." | ||
|
||
WORK_PACKAGE_MESSAGE_TEMPLATE = ( | ||
"**{type}** work package **{subject}** was {action} by **{author}**." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might revise this to "**{subject}** (work package **{type}**) was {action} by **{author}**"
.
Current:
Task work package Draft project description was created by Nirved Mishra.
Revised:
Draft project description (work package Task) was created by Nirved Mishra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
zerver/webhooks/openproject/view.py
Outdated
) | ||
|
||
ATTACHMENT_MESSAGE_TEMPLATE = ( | ||
"**{author}** uploaded **{filename}** in {container_type} **{container_name}**." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that in the generated fixture for this type of event, the container_type
renders to "workpackage", which looks a bit odd since their docs and our other message templates use "work package". It looks like the type is in camel case ...
"container": {
"_type": "WorkPackage",
Maybe we just omit that and have the name of the container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can ignore this since adding an extra function to format camel case to regular case is unnecessary.
zerver/webhooks/openproject/view.py
Outdated
) | ||
|
||
TIME_ENTRY_MESSAGE_TEMPLATE = ( | ||
"**{user}** {action} a time entry of **{hours}**{workpackage_message}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the hours
here looks to be the duration string of ISO 8601 (https://en.wikipedia.org/wiki/ISO_8601#Durations); see https://www.openproject.org/docs/api/endpoints/time-entries/#local-properties and https://www.openproject.org/docs/api/basic-objects/#dates-times-durations-and-timestamps.
It'd be nice to have this time duration be more readable in the Zulip message as it's not super clear that the below is noting that Nirved logged spending 1 hour of time on the work package task.
Current:
Nirved Mishra created a time entry of 1H for Create project timeline.
Maybe revise to something like ...
Nirved Mishra logged 1 hour on Create project timeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes created a function that uses regex to parse the iso duration to human readable time.
20c94d4
to
e7f7b1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @theofficialvedantjoshi, quick reminder to use rebase instead of merge.
I've requested you to look into the rebase workflow twice before, please make sure to check it out, and stick to it.
Note: Though I've left a suggestion, I have not actually reviewed or verified any of the changes.
zerver/webhooks/openproject/view.py
Outdated
if duration is None: # nocoverage | ||
raise ValueError(f"Invalid ISO 8601 duration format: {iso_duration}") | ||
|
||
days = int(duration.group("days") or 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lines 109-122 look redundant to me.
Can you check if something like the below works instead?
days = int(duration.group("days") or 0) | |
time_units = ['days', 'hours', 'minutes', 'seconds'] | |
formatted_duration = [ | |
f"{int(duration.group(unit))} {unit[:-1] if int(duration.group(unit)) == 1 else unit}" | |
for unit in time_units if duration.group(unit) | |
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this works just fine. I thought my code would be better for readability but this is much more concise.
9915117
to
c606e72
Compare
@Niloth-p Thanks for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theofficialvedantjoshi - Thanks for making the updates for the time entry duration. I think we can improve those event messages a bit more. Let me know if you have any questions about my comments!
zerver/webhooks/openproject/view.py
Outdated
user=action_data["_embedded"]["user"]["name"].tame(check_string), | ||
) | ||
) | ||
topic = action_data["_embedded"]["project"]["name"].tame(check_string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe when there's no workpackage, we add "for this project" to the message? Hmmm... I guess if they've set the topic in the webhook URL, then that would be misleading. Their docs do say that the project will be not null for a time entry: https://www.openproject.org/docs/api/endpoints/time-entries/#linked-properties.
I think that if there's no work package, then adding the project name to the message makes sense ...
Current:
Nirved Mishra logged 1 hour.
Revised:
Nirved Mishra logged 1 hour on Project1.
That way if the topic is set by the bot's URL, there's still information about where the time entry was logged.
18be545
to
a727ad8
Compare
Fixes zulip#29944. Co-authored-by: theofficialvedantjoshi <[email protected]> Co-authored-by: Niloth P <[email protected]>
a727ad8
to
a3a9dca
Compare
@theofficialvedantjoshi - Thanks for making those updates! I had a few small notes left, so I went ahead and updated the changes here for those, see below. I''m going to mark this as ready for integration review, though @Niloth-p feel free to comment on anything if you do see any Updates:
Notes:
Updated doc screenshot: |
I think it's probably OK to use |
Merged, thanks everyone for the effort on this! |
Fixes: #29944
Screenshots and screen captures:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: