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

Add projectstatuses method to get project statuses (missing endpoint) #1267

Merged
merged 9 commits into from
Feb 10, 2023

Conversation

kayx23
Copy link
Contributor

@kayx23 kayx23 commented Dec 31, 2021

Added projectstatuses method for /project/{projectIdOrKey}/statuses

Relevant issue: #723

Atlassian Doc for this REST endpoint:

@adehad adehad requested review from adehad and studioj and removed request for adehad January 3, 2022 10:26
@adehad adehad added the feature label Jan 3, 2022
@adehad adehad linked an issue Jan 3, 2022 that may be closed by this pull request
adehad
adehad previously approved these changes Jan 3, 2022
Copy link
Contributor

@adehad adehad left a comment

Choose a reason for hiding this comment

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

Looks good to me ! Will wait a bit too see if @studioj spots anything I've missed.

Thanks !

jira/client.py Outdated
@@ -2927,6 +2927,19 @@ def statuses(self) -> List[Status]:
]
return statuses

def projectstatuses(self, projectIdOrKey: str) -> List[Status]:
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking)

@studioj what are you thoughts on using the more conventional naming convention here?

e.g.:

Suggested change
def projectstatuses(self, projectIdOrKey: str) -> List[Status]:
def project_statuses(self, projectIdOrKey: str) -> List[Status]:

My only concern is that we don't really conform to this anyway, e.g. statuscategories() below. So perhaps we want to have a separate merge request where we add a deprecation warning to the current non snake_case methods, and also convert them to a wrapper of the snake case ones. Something like:

def projectstatuses(self, projectIdOrKey: str) -> List[Status]:
    warnings.warn("Please use project_statuses()", DeprecationWarning)
    return self.project_status(projectIdOrKey)
   

def project_statuses(self, projectIdOrKey: str) -> List[Status]:
    ... 

Copy link
Collaborator

Choose a reason for hiding this comment

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

i would indeed do that in a separate PR, but tbh if we add something new i would suggest making it snake case yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

so this method should be snake case
project_statuses

Copy link
Collaborator

@studioj studioj left a comment

Choose a reason for hiding this comment

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

cool thank you for your addition.
We do prefer PR's where there is a test added so the functionality can keep on running properly

jira/client.py Outdated
@@ -2927,6 +2927,19 @@ def statuses(self) -> List[Status]:
]
return statuses

def projectstatuses(self, projectIdOrKey: str) -> List[Status]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would indeed do that in a separate PR, but tbh if we add something new i would suggest making it snake case yes

jira/client.py Outdated
@@ -2927,6 +2927,19 @@ def statuses(self) -> List[Status]:
]
return statuses

def projectstatuses(self, projectIdOrKey: str) -> List[Status]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this method should be snake case
project_statuses

@kayx23
Copy link
Contributor Author

kayx23 commented Jan 17, 2022

Apologize for the back and forth... Please review again. I realized I misunderstood what the endpoint returns when writing the test case. I thought it returns all statuses within a project, but it actually returns issue types with statuses within each issue type. I've made some revisions.

CI test for servers passed... cloud failed because Test Manager setUp failed. Was this expected?

@studioj
Copy link
Collaborator

studioj commented Jan 17, 2022

CI test for servers passed... cloud failed because Test Manager setUp failed. Was this expected?

seems like this is for all PR's i hope @adehad will have the time to check... i'm not that knowledgeable with the cloud CI part.

seems like an ENV var isn't set. That part has been added pretty recent so maybe it still contains some edge cases

@adehad
Copy link
Contributor

adehad commented Jan 18, 2022

Sorry yes regarding the Cloud CI, currently it doesn't play nicely with merge requests from forks (it doesn't pass the credentials properly even though we think we've set all the right settings to allow this!)

@studioj from my perspective if the Server tests pass that is good, then on our end we can raise a PR from a branch within this repo and we should be able to activate this test to run on Cloud with the @allow_on_cloud decorator, and hopefully we should see no problems and can merge it.

@studioj
Copy link
Collaborator

studioj commented Jan 18, 2022

It's a start😀
Better late than not and of course safety first

jira/client.py Outdated
Comment on lines 2930 to 2931
def project_statuses_by_issue_type(self, projectIdOrKey: str) -> List[IssueType]:
"""Get a list of issue types available within the project with available statuses within each issue type, as each project has a set of valid issue types and each issue type has a set of valid statuses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @kayx23 !

Sorry I'm just reviewing this again and think this function name and docstring could be clarified.

Technically this function is returning issue types rather than project statuses.

So perhaps the function name should be issue_types_for_project()

I also think the short summary for the function could be made clearer with an example.

Perhaps we can modify this like so:

Suggested change
def project_statuses_by_issue_type(self, projectIdOrKey: str) -> List[IssueType]:
"""Get a list of issue types available within the project with available statuses within each issue type, as each project has a set of valid issue types and each issue type has a set of valid statuses.
def issue_types_for_project(self, projectIdOrKey: str) -> List[IssueType]:
"""Get a list of issue types available within the project.
Each project has a set of valid issue types and each issue type has a set of valid statuses.
The valid statuses for a given issue type can be extracted via: `issue_type_x.statuses`

We can then probably also update the docstring of statuses() to point to this function to help people find this use case!

        """Get a list of all status Resources from the server.

           Refer to :py:meth:`JIRA.issue_types_for_project` for getting statuses
           for a specific issue type within a specific project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! This makes a lot of sense. I've added them in.

@kayx23 kayx23 requested a review from adehad March 11, 2022 04:20
@adehad adehad enabled auto-merge (squash) February 10, 2023 09:58
@adehad adehad disabled auto-merge February 10, 2023 10:16
@adehad adehad merged commit d717272 into pycontribs:main Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get statuses that relates to a specific project
3 participants