Bug 1460345 - Sort payload and extras on task inspector

[Gabbyjose]

Created a 'sort' function on the TaskDetail component where you pass in the payload obj, sort it alphabetically by keys, and send that through JSON.stringify. Please let me know if you have any questions or any fixes I could make!

[Gabbyjose]

Hi Eli,

Thanks so much for the quick feedback! I’ll work on these changes now and let you know if I run into any trouble.

Best, Gabby

On May 11, 2018, at 11:57 AM, Eli Perelman notifications@github.com wrote:

@eliperelman requested changes on this pull request.

Thanks for the patch! A few notes:

You have a package-lock.json file committed here, but we use the Yarn package manager https://yarnpkg.com/ for managing dependencies. This mean once you have yarn installed, you can just run yarn in the root of the repo to have the dependencies installed. If you could remove this package-lock.json file from your commit that would be great. Next, it looks like your editor may have inserted some of its own spacing changes with your commit. Luckily the repo comes with a linting command to fix those, so you can run yarn lint --fix in the root and it should fix those up. I think it would be useful to sort these objects deeply, since the use case of the bug in question here most likely would want nested properties to also be in alphabetical order. That said, having to write this logic could get a little complicated, so in the past I used the deep-sort-object https://www.npmjs.com/package/deep-sort-object to do this. You can install it with yarn using yarn add deep-sort-object, then it should be as simple as sort(payload) or sort(task.extra). Give these a shot and let me know how it goes!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/taskcluster/taskcluster-tools/pull/528#pullrequestreview-119488009, or mute the thread https://github.com/notifications/unsubscribe-auth/AYNCeopojypyXANhzjc0tv_dAatU7jyHks5txbTagaJpZM4T7rLC.

[Gabbyjose]

Hi both,

Thank you for the guidance! I imported the library and passed in the payload and task.extra up there and pushed my changes up to my branch. I also ran the yarn linter, seems to fix everything except for a couple of lines around the JSON.stringify.

Let me know how this looks, and thanks again for the help!

On May 11, 2018, at 12:03 PM, Hassan Ali notifications@github.com wrote:

@helfi92 commented on this pull request.

Awesome first patch! +1 to @eliperelman https://github.com/eliperelman recommendations.

In src/views/UnifiedInspector/TaskDetails.jsx https://github.com/taskcluster/taskcluster-tools/pull/528#discussion_r187657253:

 const { error } = this.state;

• const { metadata } = task;
• const { metadata, payload } = task; Instead of calling sortPayload twice below, you can sort here and store the result in a variable.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/taskcluster/taskcluster-tools/pull/528#pullrequestreview-119487777, or mute the thread https://github.com/notifications/unsubscribe-auth/AYNCeqxbrf_sCk7zP93KMtMolgoZjkprks5txbYzgaJpZM4T7rLC.

[Gabbyjose]

Thank you!!

On May 11, 2018, at 3:37 PM, Eli Perelman notifications@github.com wrote:

Merged #528 https://github.com/taskcluster/taskcluster-tools/pull/528.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/taskcluster/taskcluster-tools/pull/528#event-1622642769, or mute the thread https://github.com/notifications/unsubscribe-auth/AYNCevukXX207P-JgEF6RbU-7sIpRCqnks5txeh4gaJpZM4T7rLC.