-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: api endpoints for projects #28111
base: main
Are you sure you want to change the base?
Conversation
@dineshsalunke Is the PR still WIP or is it ready for review? |
@denyskon this one is ready for review |
looks good to me! |
@dineshsalunke I see a lot of unrelated formatting changes, maybe originating from your editor formatter or similar. Could you revert them? |
@denyskon I have fixed the golines formatting and pushed. I will still check on it and get back |
@dineshsalunke Thank you, however unfortunately some are still remaining in |
Also, some methods seem to have broken when updating to main. Could you take a look please? |
I think we need a project scope. If one can write issues, he can delete/edit issues/comments. If he has read permission of project, it can visit project columns UI. I don't think they are conflicted. |
👍 will update the PR accordingly. |
} | ||
return | ||
} | ||
if project.Title != form.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.
Shouldn't form Title and Description be *string and be updated only when provided? At least this is how normaly PATCH works. And I don't see any point in comparing with existing project values
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 agree with you on the first point to check if the title or description is present,
As for the second point, the reason behind comparing with existing value is that if there is some logic somewhere down the code which reacts to the changes and if there is logic of diffing for changes then setting the value here blindly would cause unwanted side effects.
I might be thinking too much on this, but its practice i picked up from other places where this was an issue.
There's a large chance that I've overlooked it, but after a quick once-over of the file changes, I don't see a way to attach a ticket to a project over the API? Neither at creation, or thereafter. I just ran into this today while I was working on some personal projects, which brought me here, so I thought I'd mention it just in case it was overlooked. Thanks for all the great work! Love the project! |
If you don't mind, I think maintainers can help update the pull request. |
services/convert/project.go
Outdated
Closed: project.ClosedDateUnix.AsTime(), | ||
} | ||
|
||
_ = project.LoadRepo(ctx) |
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 return error should be handled.
@lunny I noticed that the board related models have been renamed to Column and Template, so do we wanna update all the properties in the payloads and responses accordingly ? If we change them wouldn't that be a breaking change |
At the moment, I prefer to change the names as more as possible but keeping compatible. You can find I didn't change the table name and some places in my original PR. |
// required:true | ||
Title string `json:"title" binding:"Required"` | ||
// required:true | ||
BoardType uint8 `json:"board_type"` |
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.
TemplateType to align with other places?
m.Combo("/{id}").Get(projects.GetProject). | ||
Patch(bind(api.UpdateProjectPayload{}), projects.UpdateProject). | ||
Delete(projects.DeleteProject) | ||
}, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryIssue), reqToken()) |
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 think we need a new token scope AccessTokenScopeCategoryProject
.
Fixes #14299
My earlier PR #20208 went too long and became messy, so I am closing that one and will be splitting the work in smaller chunks.
This PR includes all the endpoints for the Projects
For all the endpoints related to Boards I will be raising another PR for the same.