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

Jeff schneider add powercomman sdkv3 #62

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jjschnei
Copy link
Contributor

@jjschnei jjschnei commented Feb 3, 2024

No description provided.

const tasks = await taskProvider.getTaskRefs(tasksApiInstance, projectId);

for(let task of tasks.data) {
if (filters.some(filter => !filter.matchesTask(task))) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a continue rather than return it doesn't make sense to return in this for loop. You want to continue the loop if the task does not match the filter.

The return makes sense in the previous code since it's a tasks.stream().on(...) takes in a function https://github.com/Asana/devrel-examples/blob/master/javascript/powercommands/asana_power.js#L69-L73

Copy link
Contributor

@jv-asana jv-asana Feb 3, 2024

Choose a reason for hiding this comment

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

If you try to run asana-power complete --assignee "Kenan Kigunda" this won't work because the tasks being returned is missing the assignee.name. even though you requested for assignee in opt_fields in your task_provider.jshttps://github.com/Asana/devrel-examples/pull/62/files#diff-c55db41f9d5a798d2282e92481cd6de3642c3ebb4d25c7bb1d48de8ae51315a7R4

The Asana API does not return the full assignee object so it'll error when it tries to filter. Pretty sure this was broken in the v1 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the extra check. It could be worth coming back in the future if we get meaningful usage to add another request to fetch each task with getTask to get the assignee name. Could also maybe do it with assignee gid.

key: 'assignee',
value: null,
matchesTask: function(task) {
return task.assignee && task.assignee.name.includes(this.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to add another check in case the task has an assignee but no assignee.name. This was probably broken before as well since client.projects.tasks(projectId); does not return the full task object with assignee
return task.assignee && task.assignee.name.includes(this.value); ->
return task.assignee && task.assignee.name && task.assignee.name.includes(this.value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this

const getTaskRefs = async (tasksApiInstance, projectId) => {
console.log('Retrieving all task references for project', projectId);
let opts = {
opt_fields: 'name,assignee,completed,resource_type,resource_subtype'
Copy link
Contributor

Choose a reason for hiding this comment

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

The assignee filter and notes filters won't work unless you ask for those properties in opt_fields

FROM:

    opt_fields: 'name,assignee,completed,resource_type,resource_subtype'

TO:

    opt_fields: 'name,assignee.name,completed,resource_type,resource_subtype,notes'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the opt_fields to: 'name,assignee.name,completed,resource_type,resource_subtype,notes'

@jv-asana
Copy link
Contributor

jv-asana commented Feb 6, 2024

NIT:

To be consistent with powercommands we might want to rename the folder that contains this code from powercommand_using_SDKv3 to powercommands_using_SDKv3 -> The difference here being the "s". This makes it easier to hit tab on the keyboard to select the folder in the CLI

Add assignee and notes to opt_fields so those filters work.
@jjschnei
Copy link
Contributor Author

I'll update the folder name (i.e. add an s to powercommand) on a subsequent commit once these changes are merged.

@jv-asana
Copy link
Contributor

I'll update the folder name (i.e. add an s to powercommand) on a subsequent commit once these changes are merged.

why not send a commit for it now?

@jjschnei
Copy link
Contributor Author

I'll update the folder name (i.e. add an s to powercommand) on a subsequent commit once these changes are merged.

why not send a commit for it now?

My mac is in a weird state with github and mac_configure doesn't work to fix it. I'm underwater with what came in while I was out so can't dedicate the time at the moment to try and fix it. This means I can't checkout the pull request to make the name change. It's an easy fix to make in the xcode web editor once the changes are merged. I just tried to change it in the github website by changing the path of the license. This kind of worked but will take more manual work now to rename all of the paths for all of the files and create a commit for each of them.

@jv-asana
Copy link
Contributor

I'll update the folder name (i.e. add an s to powercommand) on a subsequent commit once these changes are merged.

why not send a commit for it now?

My mac is in a weird state with github and mac_configure doesn't work to fix it. I'm underwater with what came in while I was out so can't dedicate the time at the moment to try and fix it. This means I can't checkout the pull request to make the name change. It's an easy fix to make in the xcode web editor once the changes are merged. I just tried to change it in the github website by changing the path of the license. This kind of worked but will take more manual work now to rename all of the paths for all of the files and create a commit for each of them.

Ok, for now let's remove the extra powercommands_using_SDKv3 you added to avoid confusion when this PR gets merged.
powercommands_using_SDKv3

@jv-asana
Copy link
Contributor

Can you, please add a .gitignore file like in the original powercommands app: https://github.com/Asana/devrel-examples/blob/master/javascript/powercommands/.gitignore

@jv-asana
Copy link
Contributor

jv-asana commented Feb 13, 2024

Not sure how important this is but we're also missing a LICENSE file like the original powercommands: https://github.com/Asana/devrel-examples/blob/master/javascript/powercommands/LICENSE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants