Enhanced Skills Activity GHA to use Skills Directory

[xnealcarson]

Fixes #8316

What changes did you make?

• added most recent skillsIssueNums.csv to the github-actions/utils/_data folder, and converted the directory to JSON format (skills-directory.json).
• Created a new module in utils that looks up in the directory and return the information (skills-directory.js).
• added import module to post-to-skills.js for the skills directory
• Refactored code in post-to-skills-issue.js to search the newly added skills directory before deferring to the querySkillsIssue() GitHub API, if necessary, upon which new info found using querySkillsIssue() will be saved to the skills directory
• Tested changes by creating fake skills issue on my repo's project board (https://github.com/xnealcarson/website/issues/17) and then creating subsequent test issue to ensure proper function of Skills Activity tracking (https://github.com/xnealcarson/website/issues/18)

Why did you make the changes (we will use this info to test)?

• We needed to reduce the number of unnecessary GitHub API calls that the "Member Activity Trigger" workflow makes when posting to a user's Skills Issue by providing a user directory. New info found

CodeQL Alerts

After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.

Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown

Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.

• [ ] I have checked this PR for CodeQL alerts and none were found.
• [x] I found CodeQL alert(s), and (select one):
  • [x] I have resolved the CodeQL alert(s) as noted
  • [ ] I believe the CodeQL alert(s) is a false positive (Merge Team will evaluate)
  • [ ] I have followed the Instructions below, but I am still stuck (Merge Team will evaluate)

Instructions for resolving CodeQL alerts

If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.

In general, CodeQL alerts should be resolved prior to PR reviews and merging

Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)

No visual changes to the website

NOTE TO REVIEWERS

If you would like to test these changes for yourself:

• [ ] Make sure your HACKFORLA_GRAPHQL_TOKEN is active
  • [ ] Change the default branch of your repo to a test branch
  • [ ] Change line in activity.trigger.yml to refer to your own repo.
  • [ ] Add a Skills Issue in your name to your project and assign it to yourself. Take note of the issue number and add it below.
  • [ ] The automation uses GraphQL queries to locate each user's Skills Issue and the bot comment. HfLA's project, status, and field ids are hard-coded in the file github-actions/utils/_data/status-field-ids.js, so you will need to tell the bot the number of your repo's Skills Issue by making the following edits:
    • [ ] In post-to-skills-issue.js, change:

              const isActiveMember = await checkTeamMembership(github, context, eventActor, TEAM);
      

      to:

              // const isActiveMember = await checkTeamMembership(github, context, eventActor, TEAM);
              const isActiveMember = true;
      

    • [ ] Change lines 39-44 in post-to-skills-issue.js to:

        // Get eventActor's Skills Issue number, nodeId, current statusId (all null if no Skills Issue found)
        // const skillsInfo = await querySkillsIssue(github, context, username, SKILLS_LABEL);
       const skillsIssueNum = _the number of the skills issue you created_ ;  
       const skillsIssueNodeId = null;
       const skillsStatusId = null;
       const isArchived = false;
      

[github-actions[bot]]

Want to review this pull request? Take a look at this documentation for a step by step guide!

---

From your project repository, check out a new branch and test the changes.

git checkout -b xnealcarson-enhance-gha-skillsactivity-8316 gh-pages
git pull https://github.com/xnealcarson/website.git enhance-gha-skillsactivity-8316

[ryanfkeller]

Hi @xnealcarson, I'll give this a review as well! ETA 10/17, generally able to be available 1-5p weekdays.

[ryanfkeller]

Hey @xnealcarson, thanks for taking this issue! It looks like you're on the right track, and also great job on the PR description -- it made it easy to understand what you've done here.

I noticed a couple things in my review:

• [x] In post-to-skills.js, I think you have some old code on line 55 that creates a syntax error on trying to re-define skillsInfo (and if it ran would result in an unneeded re-query for the skills issue). That line can probably just be deleted. Nitpick but the extra indentation on the following lines could probably also be removed.
• [x] In post-to-skills.js, I can see that we are using a query to search for the "activity comment" even when we got the issue number from the local JSON. Since I believe the JSON includes the comment ID under the "id" category, you should be able to just read/patch that comment directly and skip the search if there is a JSON entry. The query/search you have should definitely still be a fallback for when you don't have the issue info saved locally.
• [x] Since we do save the "activity comment" id in the skills-directory.json, we'll want to make sure to update that JSON with the ID when we have to create a new "activity comment".
• [x] We may not want to save the skillsIssueNums-*.csv in the _data directory, since it isn't used directly and with this flow in action, I expect the .json should be the source of truth.
• [ ] (Edited, I missed some things in your description in my original comment, sorry!) Instead of changing the post-to-skills-issue.js to bypass isActiveMember and skillsInfo collection, would it be possible to instead modify our local copy of the _data sources so the full script flow is tested? We probably also want the test case of the skills issue not existing in _data as well so the query is executed. If you have examples of that, could you share the workflow run link?

Again, really nice work! Looking forward to seeing the next rev :rocket:

[t-will-gillis]

Hey @xnealcarson Great work on this so far!!! Thanks for being patient.

I am still looking through things but have a couple comments:

• I uploaded a new file skillsIssueNums-10-19.csv that might be easier to work with than the previous one. It has what should be the correct nodeId for each issue number, and the commentId is clearer now.
• I was looking at the update-directory.js file- I don't think it is pulling up the info correctly, maybe use .filter sim to this: const result = directory.filter(entry => entry.username === eventActor); and return result ?
• Since the .csv won't provide the skillsStatusId or isArchived info, and since the querySkillsIssue() won't find a comment id, adding in some default values like this will be helpful later on:

  const skillsIssueNum = skillsInfo.issueNum;
   const skillsIssueNodeId = skillsInfo.issueId;
   const skillsStatusId = skillsInfo?.statusId || 'unknown';
   const isArchived = skillsInfo?.isArchived || false;
   const commentFoundId = skillsInfo?.commentId || null;   // not used currently
  
   console.log(`skillsIssueNum: ${skillsIssueNum}, skillsIssueNodeId: ${skillsIssueNodeId}, skillsStatusId: ${skillsStatusId}, isArchived: ${isArchived}`);  // only for debugging  
  

  This should (I believe) let the code run after line 116 +/-
• This code above defines the commentFoundId - which is the comment_id where the bot is adding it's comments.
• A warning however: when I was first looking at this ER I assumed that the comment id was all we needed to know to update the comment, but I am thinking now that this does not help us a lot because even though we have the comment_id we need to retrieve the body of the comment before we can PATCH it. That is not at all helpful to you I know. If you and/or Ryan have ideas..?

Otherwise, I'm still looking at this but hopefully these comments and @ryanfkeller 's are helpful! Great job again!

[DDVVPP]

Hi @xnealcarson! Just checking in on the status of the PR. It looks like you made some recent changes - just a reminder to click on the 'Re-request review' icon next to the reviewers name (on the top right panel) if it's ready for review.

[xnealcarson]

Thanks for the check-in @DDVVPP. I just picked up last night where I left off, after getting a bit sidetracked this past week. I really let time get ahead of me.

I'm about wrapped up with requested changes, save for a couple things, so I am not quite ready to send out those re-requests. I will check in again with another status update (hopefully the last!) tomorrow after 3pm PST. So sorry for the delay and thank your for your patience @ryanfkeller and @t-will-gillis.

[xnealcarson]

Ran into some issues working on the last of each of your requested changes, which I Slack'd @t-will-gillis about. He aims to assist me with those tomorrow, so I will give another follow-up then once I've received his feedback!

[t-will-gillis]

Hey @xnealcarson Thanks and sorry for mixing up the function names. As I mentioned in Slack, the file I meant is utils/skills-directory.js and the function is lookupSkillsDirectory(). What I was meaning to say is that from what I can tell, return directory[eventActor] is returning a null and that you would need to lookup the directory info a different way- using find() (I said .filter() but find is more appropriate).

I threw many console.log() s into the function to show what is happening:

function lookupSkillsDirectory(eventActor) {
    const directory = loadDirectory();
    console.log(``);
    console.log(`At lookupSkillsDirectory() in skills-directory.js :`);
    console.log(`eventActor: ${eventActor}`);
    console.log(`directory[eventActor]: ${directory[eventActor]}`);
    console.log(``);
    
    const result = directory.find(entry => entry.eventActor === eventActor);
    if (result) {
      console.log(`result: ${JSON.stringify(result)}`);
      console.log(`result["issueNum"]: ${result["issueNum"]}`); // just to verify
    }
    console.log(``);
    console.log(`return 'result' rather than 'directory[eventActor]`);
    console.log(``);
    
    // return directory[eventActor] || null;
    return result || null;

If you sub these temporary log messages in, I think result is what you are looking for, and something similar would also be true for updateSkillsDirectory().

Thanks again for sticking with this!

[xnealcarson]

Hey @ryanfkeller and @t-will-gillis . My apologies for the delay in implementing your requested changes. I've just added Will's suggested changes for utils/skills-directory.js and am ready to re-request your reviews for all the changes that I have made. If there are any additonal edits that need to be made outside of these initial changes, I will be sure to deliver them more promptly in an effort to close out this issue before the approaching Thanksgiving and December breaks. Many thanks for your time and patience. I will be on the lookout for your second reviews!

[t-will-gillis]

Hi @xnealcarson echoing what @ryanfkeller said- great work!

Here are some comments in addition to Ryan's:

• I may have mislead you with the code I left in my last comment. I was using temporary console.log()s to help with troubleshooting lookupSkillsDirectory(), but as Ryan notes it will be good to clean up / remove the logs for the commit.
• Previously, the first line in lookupSkillsDirectory() was: const directory = loadDirectory();. If you add this back it addresses Ryan's comment that loadDirectory() is not being used, as well as the comment that directory is not initialized.
• (need const directory in updateSkillsDirectory() also?)
• As Ryan notes- it looks like lookupSkillsDirectory() is missing the closing bracket. (This would clear up one of the CodeQL alerts also)
• Agree with Ryan's note that commentFoundId should be commentIdCached
• I think that the updateSkillsDirectory() might need some work still

Thanks for working on this! Getting closer

[xnealcarson]

Hey @ryanfkeller and @t-will-gillis ! Sorry for the delay and many thanks for the detailed feedback. I did find your inline comments to be very helpful @ryanfkeller, along with your supporting comments @t-will-gillis.

We seem to be getting closer to wrapping up indeed with the changes I've made. As I'm writing this comment though, I see the CodeQL alert about the unused result variable in updateSkillsDirectory, which I believe has been triggered by my addition of the const directory at the top of that respective function, per your tentative suggestion in your latest comment @t-will-gillis . Should I not have added const directory in updateSkillsDirectory` then?

Other than that though, I will be on the lookout for any further suggestions you may have. Thanks again to both of your for your patience and help with this issue!!

[t-will-gillis]

Hey @xnealcarson Here is a heads up before we talk on Zoom:

• The skills-directory.json file that I gave you has the wrong kind of issue ids. My bad of course. Two solutions: one is to clear the file completely and let your function fill in the values as needed, and the second is for me to generate a new file. (FYI I discovered that GitHub elements such as issues use different ids at different times. I can explain the reason if you want, but to keep it short, the ids currently shown in the JSON file will cause an error later in this function.)
• In post-to-skills-issue.js, I think that the call to updateSkillsDirectory() will need to happen later in the file, after finding the id of the comment with the MARKER, and not in the if (!skillsInfo) loop. Something you could do is trigger a flag in the loop to run the update after the MARKER comment is found.
• Also, the parameters for updateSkillsDirectory() need to match up with the four values in the skills-directory.json.
• Finally, I previously mentioned that the updated skills-directory.json needs to be committed so that the changes are actually saved. There is an action by "stefanweifel" that we use in a few places for this reason. Check out schedule-monthly.yml line 42 onward- this will let us save the updated JSON.

Again, thank you for keeping on this