Want to review this pull request? Take a look atthis 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-pagesgit pull https://github.com/xnealcarson/website.git enhance-gha-skillsactivity-8316

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

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:

• Inpost-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.
• Inpost-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.
• Since we do save the "activity comment" id in theskills-directory.json, we'll want to make sure to update that JSON with the ID when we have to create a new "activity comment".
• We may not want to save theskillsIssueNums-*.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 thepost-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 🚀

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 fileskillsIssueNums-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 theupdate-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); andreturn result ?
• Since the .csv won't provide theskillsStatusId orisArchived info, and since thequerySkillsIssue() 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 thecommentFoundId - which is thecomment_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 thecomment_id we need to retrieve thebody of the comment before we canPATCH 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!

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.

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.

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!

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

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

functionlookupSkillsDirectory(eventActor){constdirectory=loadDirectory();console.log(``);console.log(`At lookupSkillsDirectory() in skills-directory.js :`);console.log(`eventActor:${eventActor}`);console.log(`directory[eventActor]:${directory[eventActor]}`);console.log(``);constresult=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;returnresult||null;

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

Thanks again for sticking with this!