-
-
Notifications
You must be signed in to change notification settings - Fork 955
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(integration): import github labels as tags #3348
base: master
Are you sure you want to change the base?
Conversation
- adjust GithubIssue models to include a label array - moves the label element from GithubIssue to GithubIssueReduced (indirectly in GithubIssue still through extension) (`github-issue.model.ts`) - get them from the graphQL API (`gihub-issue-map.util.ts`) - add labels with a private helper function as tags (with import of name and color) using TagService utilities (`gihub-common-interfaces.service.ts`) part of the implementation regarding johannesjo#2866
- add config member for a flag to import labels as tags or not (`github.model.ts`) - set default for that config member to false and add to UI form (`github.const.ts`) - add const elements for that form to be filled in with translation elements/values (`t.const.ts`) - first formulations of form elements in english and german (`en.json`, `de.json`) part of the implementation regarding johannesjo#2866
This did not work in my dev server yet, but this way it might be easier to find solutions together :) |
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.
Looks very clean so far! I added a few remarks.
@@ -174,4 +181,12 @@ export class GithubCommonInterfacesService implements IssueServiceInterface { | |||
private _isIssueDone(issue: GithubIssueReduced): boolean { | |||
return issue.state === 'closed'; | |||
} | |||
|
|||
private _getGithubLabelsAsTags(labels: GithubLabel[]): 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.
I think, I'd take a different approach, since there is already a mechanism for adding data to thew new task in getAddTaskData
. I'd add the additional data for the tags there and then use this info inside IssueService.addTaskWithIssue
to trigger creating the tags there. This way we can use the same mechanism for other issue providers.
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 afraid I don't really get what you're saying here.. The way this is implemented, it does trigger through the IssueService.addTaskWithIssue
function as the tags are imported as getAddTaskData
function, right? And another Provider would need to support and implement such a functionality according to their API and restrictions anyway right? Could you outline a little more, where/how you'd like the logic to be settled?
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.
Still open (I don't know what mechanism you are referring to that is not already triggered/in the pipeline with the implemented approach)
@@ -97,6 +98,14 @@ export const GITHUB_CONFIG_FORM: LimitedFormlyFieldConfig<GithubCfg>[] = [ | |||
label: T.F.GITHUB.FORM.IS_ASSIGNEE_FILTER, | |||
}, | |||
}, | |||
{ | |||
key: 'importLabelsAsTags', |
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.
For booleans the current code base almost always uses an is
prefix e.g. isSomethingTrue
.
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.
addressed in 67e18a6
Unrelated to the current review stage: I poked around to due to the requested changes and realized I forgot to implement a query to check, if the tags should be imported. I'll try to mimic analogous problems (e.g.
Also, with a similar status/takeaway, though it might be helpful, if someone else could do this as well: I still cant get the config to work (neither dev server nor prod page as web app under Linux), this should also be test driven before a possible/eventual merge:
|
Yes, this is very important I think. Likely not everybody wants to import the tags.
What type of issues are you encountering? Maybe I can help? |
Well, the GitHub integration did not work in the web app. I'm not sure if that is a 'my machine' problem or more general, first I thought it had to be due to my changes but it does not work in the production web app either - the 'save' button is simply not clickable and the configuration does not seem to be saved in the latter setting screen. Unfortunately I haven't been able to investigate this further yet on my machine or a reference windows device. |
That sounds not like it should be :) Can you maybe open up an issue for the problems you're encountering and what steps lead you there (also important to know the browser and environment you are working with)? |
Will do once I find a moment to formalize exactly that! :) |
Description
adjust GithubIssue models to include a label array -
moves the label element from GithubIssue to GithubIssueReduced (indirectly in GithubIssue still through extension) (
github-issue.model.ts
)get them from the graphQL API (
gihub-issue-map.util.ts
)add labels with a private helper function as tags (with import of name and color)
using TagService utilities (
gihub-common-interfaces.service.ts
)add config member for a flag to import labels as tags or not (
github.model.ts
)set default for that config member to false and add to UI form (
github.const.ts
)add const elements for that form to be filled in with translation elements/values (
t.const.ts
)first formulations of form elements in english and german (
en.json
,de.json
)Issues Resolved
Resolves #2866
Check List