Added the possibility to edit showcases #207

NetroScript · 2018-07-20T00:01:44Z

The option "showcases" was added to editProfile().

For this option you have to submit an array of objects. These objects have all the property "showcase", and some need the property "values". They will be displayed in the order in which they were in the array.

Possible showcase values:
infobox, artwork, trade, items, game, badge, rareachievements, screenshot, group, review, workshop, guide, achievements, games, ownguides, ownworkshop, (steam game ones like salien, but these will be obsolent at some point so not included here)

Information about these:

infobox:
- values: "title" and "notes"
artwork:
- values: a single array with up to 4 publishedfileids (f.e. just copy them from the page of your artwork)
trade:
- values: "notes"
items:
- values: none
game:
- values: a single number which is the appid
badge:
- values: "style" which can be one of the following - [rare, selected, recent, random], "badges" which is an array of objects in the following manner - {"appid": ..., "badgeid": ..., "border_color": ...} (although I do not know the influence of border_color, if the badge is not a game badge, fill only badgeid, if it is a badge from a game the badgeid is 1 and you have to fill in appid) up to 6
rareachievements:
- values: none
screenshot:
- values: a single array with up to 4 publishedfileids
group:
- values: SteamID
review:
- values: a single number which is the appid of the game where you want to display your review
workshop:
- values: "appid" of the game, "publishedfileid" of the workshop item
guide:
- values: "appid" of the game, "publishedfileid" of the guide
achievements:
- values: "achievements" which is an array of objects in the following manner - {"appid": ..., "title": ...} (for the title either get it manually or use https://steamcommunity.com/id/<Steam ID>/ajaxgetachievementsforgame/<app id>) up to 7
games:
- values: none
ownguides:
- values: an array of objects in the following manner - {"appid": ..., "publishedfileid": ...} up to 4
ownworkshop:
- values: an array of objects in the following manner - {"appid": ..., "publishedfileid": ...} up to 5

Example of usage:

community.editProfile({
    "realName": "Bot",
    "summary": "Summary",
    "showcases": [{
        "showcase": "game",
		"values": 440
    }, {
        "showcase": "achievements"
    }, {
        "showcase": "trade",
        "values": {
            "notes": "Trade notes."
        }
    }, {
        "showcase": "infobox",
        "values": {
            "title": "24/7",
            "notes": "Items: ...."
        }
    }, {
        "showcase": "games",
    }]
}, (err) => {
    if (err) console.log(err);
});

Additionally the method editShowcaseItem() was added, which takes 4 options.
These are showcase, slot, item and callback.
Currently there are 3 types of showcases which you can edit with this function (because they use the
ajaxsetshowcaseconfig endpoint). These are:

"trade"
- has 6 slots
- takes an object in the following manner as item: {"appid": ..., "item_contextid": ..., "item_assetid": ...}
"items"
- has 10 slots
- takes an object in the following manner as item: {"appid": ..., "item_contextid": ..., "item_assetid": ...}
"games"
- has 4 slots
- takes an object in the following manner as item: {"appid": ...}

Example:

community.editShowcaseItem("trade", 2, {
	"appid": 440,
	"item_contextid": 2,
	"item_assetid": 670000
}, (err) => {
	if(err){
	console.log(err)
	}
});

Some showcases of course only work if you fulfill the requirements, meaning it could be that I am missing some.

The option "showcases" was added to editProfile(). For this option you have to submit an array of objects. These objects have all the property "showcase", and some need the property "values". They will be displayed in the order in which they were in the array. Possible showcase values: infobox, artwork, trade, items, game, badge, rareachievements, screenshot, group, review, workshop, guide, achievements, games, ownguides, ownworkshop, (steam game ones like salien, but these will be obsolent at some point so not included here) Information about these: infobox: * values: "title" and "notes" artwork: * values: a single array with up to 4 publishedfileids (f.e. just copy them from the page of your artwork) trade: * values: "notes" and "items" which is an array of objects in the following manner - {"appid": ..., "item_contextid": ..., "item_assetid": ...} up to 6 items: * values: "items" which is an array of objects in the following manner - {"appid": ..., "item_contextid": ..., "item_assetid": ...} up to 10 game: * values: "appid" badge: * values: "style" which can be one of the following - [rare, selected, recent, random], "badges" which is an array of objects in the following manner - {"appid": ..., "badgeid": ..., "border_color": ...} (although I do not know the influence of border_color, if the badge is not a game badge, fill only badgeid, if it is a badge from a game the badgeid is 1 and you have to fill in appid) up to 6 rareachievements: * values: none screenshot: * values: a single array with up to 4 publishedfileids group: * values: "groupid" which is a SteamID review: * values: "appid" of the game where you want to display your review workshop: * values: "appid" of the game, "publishedfileid" of the workshop item guide: * values: "appid" of the game, "publishedfileid" of the guide achievements: * values: "achievements" which is an array of objects in the following manner - {"appid": ..., "title": ...} (for the title either get it manually or use https://steamcommunity.com/id/<Steam ID>/ajaxgetachievementsforgame/<app id> ) up to 7 games: * values: "games" which is an array of objects in the following manner - {"appid": ...} up to 4 ownguides: * values: an array of objects in the following manner - {"appid": ..., "publishedfileid": ...} up to 4 ownworkshop: * values: an array of objects in the following manner - {"appid": ..., "publishedfileid": ...} up to 5 Some showcases of course only work if you fulfill the requirements, meaning it could be that I am missing some.

The option "showcases" was added to editProfile(). For this option you have to submit an array of objects. These objects have all the property "showcase", and some need the property "values". They will be displayed in the order in which they were in the array. Possible showcase values: infobox, artwork, trade, items, game, badge, rareachievements, screenshot, group, review, workshop, guide, achievements, games, ownguides, ownworkshop, (steam game ones like salien, but these will be obsolent at some point so not included here) Information about these: infobox: * values: "title" and "notes" artwork: * values: a single array with up to 4 publishedfileids (f.e. just copy them from the page of your artwork) trade: * values: "notes" and "items" which is an array of objects in the following manner - {"appid": ..., "item_contextid": ..., "item_assetid": ...} up to 6 items: * values: "items" which is an array of objects in the following manner - {"appid": ..., "item_contextid": ..., "item_assetid": ...} up to 10 game: * values: "appid" badge: * values: "style" which can be one of the following - [rare, selected, recent, random], "badges" which is an array of objects in the following manner - {"appid": ..., "badgeid": ..., "border_color": ...} (although I do not know the influence of border_color, if the badge is not a game badge, fill only badgeid, if it is a badge from a game the badgeid is 1 and you have to fill in appid) up to 6 rareachievements: * values: none screenshot: * values: a single array with up to 4 publishedfileids group: * values: "groupid" which is a SteamID review: * values: "appid" of the game where you want to display your review workshop: * values: "appid" of the game, "publishedfileid" of the workshop item guide: * values: "appid" of the game, "publishedfileid" of the guide achievements: * values: "achievements" which is an array of objects in the following manner - {"appid": ..., "title": ...} (for the title either get it manually or use https://steamcommunity.com/id/<Steam ID>/ajaxgetachievementsforgame/<app id> ) up to 7 games: * values: "games" which is an array of objects in the following manner - {"appid": ...} up to 4 ownguides: * values: an array of objects in the following manner - {"appid": ..., "publishedfileid": ...} up to 4 ownworkshop: * values: an array of objects in the following manner - {"appid": ..., "publishedfileid": ...} up to 5 Some showcases of course only work if you fulfill the requirements, meaning it could be that I am missing some. (and changed spaces to tabs because `Tabs for indentation`)

Further testing showed that unlike how it was in my previous tests, that the first 18 elements are not always the same (and same position) and sent in every request. In my personal test only the profile_background parameter was missing, so this code adds this parameter should it be missing in the correct place. I don't know if this is a one time problem or if it is needed to validate all of the 18 first elements of the form.

Aareksio

I have doubts, a lot of doubts.

Aareksio · 2018-07-20T19:01:36Z

components/profile.js

@@ -1,6 +1,7 @@
 const Cheerio = require('cheerio');
 const FS = require('fs');
 const SteamID = require('steamid');
+const Param = require('jquery-param');


Do we really need this?

I tried the original way of doing that, but you can't have multiple profile_showcase[] as json keys, so I did it the same way as the serializeArray() output, but this didn't work as a parameter, so it needed to be submitted as a string to the function, and the code originally was only implemented for my own project, so the fastest way was using the param function of JQuery

I really would like to avoid adding a dependency for this.

Aareksio · 2018-07-20T19:02:07Z

components/profile.js

@@ -65,57 +67,413 @@ SteamCommunity.prototype.editProfile = function(settings, callback) {

 			switch(i) {
 				case 'name':
-					values.personaName = settings[i];
+					out[8] = {
+						"name": formd[8].name,


What if field order ever changes?

I thought about it, if it changes you have to change the code, but I evaluate the chance of that relatively low. That's why I decided to keep the code shorter. Originally I had a version which still had values which was an changed into an object with : instead of :.

Longer code is better than broken code.

I can add that it doesn't use the default values on the page, but creates them itself and copies over the correct values.

Aareksio · 2018-07-20T19:02:52Z

components/profile.js

 					}

 					break;

-				// TODO: profile showcases
+				case 'showcases':
+					for (var t in settings[i]) {


What does t stand for?

Type, like type of showcase.

Usually abbreviating short words like this just leads to headache later when trying to understand what's going on. You should probably use type instead of t. It's 3 more characters, but infinitely more clear.

Aareksio · 2018-07-20T19:03:08Z

components/profile.js

-				// TODO: profile showcases
+				case 'showcases':
+					for (var t in settings[i]) {
+						var showcaseconfig = [false,0,0,[]];


What does this mean?

Showcase config allows the quick reuse of ajaxsetshowcaseconfig without writing the request everytime, the first decides if such requests are made for a specific showcase, the second is the amount of request (like f.e. 10 for 10 different achievements), the third is the customization type (deciding for which type of showcase it is), the 4th is an array with the values for the single requests.

Aareksio · 2018-07-20T19:04:24Z

components/profile.js

+								});
+
+								if (settings[i][t]["values"].hasOwnProperty("items")) {
+									showcaseconfig = [true, 6, 4, settings[i][t]["values"]["items"]];


How does this work?

Only if items are submitted the requests to change single games / items / ... are done, this is a simple check to do that.

Aareksio · 2018-07-20T19:07:19Z

components/profile.js

+							case 'infobox':
+								out.push({
+									"name": "profile_showcase[]",
+									"value": "8"


Magic number (a lot of them below)

What do you mean magic number? Are you referring to the value 8? To get those values I tested all show cases on my profile and watched the submitted parameters.

A "magic number" in programming is a number with an unclear or unspecified meaning, which leads to very unreadable code and code that's difficult to understand. Usually, a good way to avoid magic numbers is by using enums.

Aareksio · 2018-07-20T19:09:06Z

components/profile.js

+									break;
+							}
+
+							if (showcaseconfig[0]) {


Wait, so it's internal variable? It'd be explained if it were to be sent to Steam, but now... 👎

Aareksio · 2018-07-20T19:14:12Z

components/profile.js

+
+									}
+
+									self._myProfile("ajaxsetshowcaseconfig", requestdata, function (err, response, body) {


Are we making HTTP requests in a loop? IIRC there's no internal throttling, so this is begging for rate limit.

I can add like 1 second delay with setTimeout should it be really needed. But I tested if it was a problem for the endpoint if you made multiple request's at once, and didn't get an error.

Aareksio · 2018-07-20T19:16:49Z

components/profile.js

+
+										if (err || response.statusCode != 200) {
+											if (callback) {
+												callback(err || new Error("HTTP error " + response.statusCode));


I'm not sure about this, but it's a callback in for loop - this will lead to it being possibly fired multiple times.

Like I said before in the comments, this is actually of a private project, so I copied the code which was used before in the library to make the requests to edit the profile. I didn't meddle with it and didn't have any repetitive calls when I was debugging my use case.

But it should be possible when setting certain showcases. If that's the case, it won't take long until we see first my bot is <doing something> n times issues...

Aareksio · 2018-07-20T19:19:16Z

components/profile.js

 			}
 		}

-		self._myProfile("edit", values, function(err, response, body) {
+		self._myProfile("edit", Param(out), function(err, response, body) {


From my understanding of jquery-param, it will do what previous syntax achieved. Isn't taht right?

I tried multiple ways to do it before, and it didn't work for me, if it actually works without Params feel free to remove it.

Aareksio · 2018-07-20T19:20:29Z

IIRC this was also discussed at some point and McKay rejected the idea of implementing showcases into module due to each being different.

DoctorMcKay · 2018-07-20T20:47:28Z

I don't think I rejected the idea of implementing profile showcases, but if it's done then it needs to be done right, in a future-proof way.

Now the array index for name for example will always be 8.

…equests The delay is 1.5 seconds

Aareksio

This method is possibly making 21 calls to _myProfile and the timeout doesn't work properly.

The callback may be called multiple times unless I miss some logic preventing it - this is generally unexpected behaviour.

We have somehow complex problem here, and so far the code is far from solving it in passable manner, at least for me. Inconsistency, rash design and misuse of language. Maybe I'm just too picky.

@DoctorMcKay I was referring from memory to comment on #143, turns out I didn't remember it quite right 🙂 Case here is not the same indeed.

@NetroScript You may be interested in checking PR mentioned above as it seems to solve part of the problem differently.

Aareksio · 2018-07-22T02:42:36Z

components/profile.js

-			values[item.name] = item.value;
+		var formd = form.serializeArray();
+		var i = 0;
+		form.serializeArray().forEach(function (item) {


Array.forEach callback is supplied with item index on 2nd parameter

Aareksio · 2018-07-22T02:44:32Z

components/profile.js

@@ -65,57 +84,401 @@ SteamCommunity.prototype.editProfile = function(settings, callback) {

 			switch(i) {
 				case 'name':
-					values.personaName = settings[i];
+					out[8].value = settings[i];


Magic numbers - there's clear relation between parameter name and it's index. Replacing them with enum should also remove the need for the whole switch statement here. I'd question first whether it really needs to be an array instead of an object.

It needs to be an array because an object can't have multiple profile_showcase[] keys.

And the switch statement is there in the original too and I assume it is because if you don't iterate the properties and instead assign them f.e. variable.city=settings.city it would require that city is defined each time it is submitted. With the current way you can leave out city to keep the old value.

Aareksio · 2018-07-22T02:48:02Z

components/profile.js

 					break;

 				case 'featuredBadge':
 					// Currently, game badges aren't supported
-					values.favorite_badge_badgeid = settings[i];
+						out[16].value = settings[i];


Aareksio · 2018-07-22T02:49:33Z

components/profile.js

+								}
+								break;
+
+								case 'games':


Indent broken here and below

Aareksio · 2018-07-22T03:01:50Z

components/profile.js

+								});
+								out.push({
+									"name": "rgShowcaseConfig[8][0][title]",
+									"value": settings[i][type]["values"]["title"]


If you have already decided to access objects properties with obj.property syntax, you should stick to it.

…m failed. With the variable `showshowcaseconfigerrors` (true or false) in the values object it is now possible to control if the callback is called when a request fails. Additionally the callback is fired only once now in any case and the error message has the string appended, that the error happened when changing a specific item. (` | Happened while updating specific showcase items.`)

Now it can't happen, that f.e. games and items are changed at the same time leading to simultaneous requests.

Additionally now all values are unchanged if not supplied, or supplied as `undefined` in arrays. Showcases where the `values` property had only one value were changed to only this value instead of an additonaly object with f.e. "appid" as key. (Showcases were this changed are `game`,`review`,`group`). Additionally changed condition for errors in `ajaxsetshowcaseconfig` so they only throw an error in the correct scenario.

Aareksio · 2018-07-22T17:02:50Z

Inconsistency, rash design and bad use of language features

It definetly looks better, but the main problems stay. At least now it doesn't change any of existing functionality.

Considering no one is likely picking up this topic anytime soon, it may probably pass - assuming it works.

NetroScript · 2018-07-22T17:43:00Z

@Aareksio

main problems stay

To which main problems are you referring here?
As far as I know I responded to all problems which you mentioned.

Aareksio · 2018-07-22T17:04:02Z

components/profile.js

-				// TODO: profile showcases
+
+				case 'showcases':
+					var num_of_requests = 0;


Consistency 😢

Aareksio · 2018-07-22T17:04:30Z

components/profile.js

+					for (var type in settings[i]) {
+						//Variable used to easily make request to`ajaxsetshowcaseconfig` for showcases like trade, items, ...
+						var showcaseconfig = {
+							"supplied": false,


Consistency

Aareksio · 2018-07-22T17:04:34Z

components/profile.js

+						};
+						//Controls if the callback function is called when a request changing f.e. a single item or game in a showcase fails
+						var errorcontrol = {
+							error: false,


Consistency

Aareksio · 2018-07-22T17:05:40Z

components/profile.js

+					}
+					for (var type in settings[i]) {
+						//Variable used to easily make request to`ajaxsetshowcaseconfig` for showcases like trade, items, ...
+						var showcaseconfig = {


This whole config could be mitigated by having separate method. 😞

Aareksio · 2018-07-22T17:06:41Z

components/profile.js

+
+				case 'showcases':
+					var num_of_requests = 0;
+					//When supplying a new showcases array, remove the old showcase (order)


Whitechars (new lines) please

Aareksio · 2018-07-22T17:08:33Z

components/profile.js

+						};
+						if (settings[i][type].hasOwnProperty("values")) {
+							if (settings[i][type]["values"].hasOwnProperty("showshowcaseconfigerrors")) {
+								errorcontrol.showerrors = settings[i][type]["values"]["showshowcaseconfigerrors"];


Consistency... obj['values']['show...something'] instead of obj.values.showsomething 🤔

Aareksio · 2018-07-22T17:10:56Z

components/profile.js

+									if (settings[i][type]["values"].hasOwnProperty("badges")) {
+										var types = ["badgeid", "appid", "border_color"];
+										for (var n = 0; n < 6; n++) {
+											for (var m = 0; m < 3; m++){


Random loop in another random loop. There's plenty of solutions to make it clear what happens here, for ... of is one of them.

Aareksio · 2018-07-22T17:12:13Z

components/profile.js

+								break;
+
+							case 'rareachievements':
+								values["profile_showcase[1]"] = 1;


At this point I start to wonder what's the purpose of index in profile_showcase... Is this the order on page? If so, shouldn't it be ordered as user wanted? What if the user is lvl 10 and has only one showcase slot?

(All that is invalid if the index doesn't have anything to do with display order)

That's why I used the Array first, until I did some further testing and found out that the index of that doesn't matter, it has no influence at all it seems. The only important thing is the order of the parameters in the request, and because I remove all profile_showcase[] if the showcases are changed and add them again to the JSON object and JSON is sorted by timestamp, it means the showcases still have the same order on the profile as in the array. If the user is level 10 and has 1 showcase slot and submits 2 showcases in the array he will get an error and the whole EditProfile Call is invalid (but changes to f.e. the items in an item showcase are still valid).

Aareksio · 2018-07-22T17:15:04Z

components/profile.js

+								if (settings[i][type].hasOwnProperty("values")) {
+									for (var n = 0; n < 4; n++) {
+										if (settings[i][type]["values"][n] != undefined) {
+											values["rgShowcaseConfig[7][" + n + "][publishedfileid]"] = settings[i][type]["values"][n];


Manual indexing here confuses me everytime.

What do you mean? n<4 instead of f.e. for(number in settings[i][type]["values"]) - I did this because there are 4 items which you can change. So the user can't supply too many now, and if he defines f.e. only 2, the other 2 are left unchanged.

You may do something like this:

settings[i][type].values.forEach((game, index) => { if (typeof game === 'undefined') return; values[`rgShowcaseConfig[7][${index}][publishfield]`] = game; }

There are still two things someone may get confused on, settings[i][type] and `rgShowcaseConfig[7][${index}][publishfield]`, but it seems to be at least slightly better.

Classic for loops are good when our focus is on index, not value.

So what if the user supplies 5 values. Nothing might happen, something might happen, who knows. Also all the code I saw was without using template strings, let, const, arrow functions, ... so I refrained from using these.

I'd say template strings are fair game, because this library only officially supports Node.js 4+

node-steamcommunity/package.json

Line 34 in b7a3e51

"node": ">=4.0.0"

which also seems a little generous and might be able to be bumped, considering 4.0 is well past its LTS.

@DoctorMcKay, you added your stats reporting library to at least node-steam-tradeoffer-manager – what are the percentages of people using various different versions of Node, assuming you record those numbers?

@NetroScript The very first word of profile.js is const. McKay have updated his libs to use templates and arrow functions (ref), seems like steamcommunity didn't get enough care though, that said, the example was about using Array.forEach instead of random loop.

So what if the user supplies 5 values. Nothing might happen, something might happen, who knows.

Well, the code is meant to be deterministic by nature, when user supplies 5 values, it will send one parameter more - what happens on Steam side is unknown indeed (it'll likely get discarded), if we know there's a restriction of 4 entries - we should somehow prevent the user from supplying more (eg. by throwing an error), instead of coercing the input.

You probably only want to use non descriptive loops when taking some sort of action multiple times, eg. sending n HTTP requests a few lines below, otherwise it leads to confusion - "Why 4? What's settings[i][type]["values"][n]?"

Aareksio · 2018-07-22T17:28:08Z

components/profile.js

+											}
+
+											if (callback && !this.error) {
+												callback(err || new Error("HTTP error " + response.statusCode + " | Happened while updating specific showcase items."));


It'll be called on first error only, but then continue working...

What do you mean?

There's if statement that checks whether this.error is falsy - you're setting it to true after first error, so callback will never be called again, but the function is already scheduled to run multiple requests despite one of them errored.

What is the problem with that? You complained before that callback is called multiple times. And the functions are independent of each other, it doesn't matter if one fails, if we have f.e. an item showcase 1 item at a time is changed, if f.e. the 5th fails all other still are valid. If you really need the items to be correct, you know one (or more) of them was faulty and can do it again, if you don't care you just ignore it.

Let me show you an example:

const showcases = { trade: { values: { notes: 'Some note', items: [item1, item2, item3] } }, items: { values: { items: [item1, item2, item3] } } }; community.editProfile({ name: 'Test', showcases }, (err, response) => { if (err) return exitWithError(err); log('Profile set up, start something'); startSomething(); });

What is going to happen?

The script will schedule 3 trade showcase HTTP calls in 1.5s, 3s and 4.5s from now, then do the same with items showcase - calls in 6s, 7.5s and 9s.

Profile edit call will be executed, it changes profile name to Test - let's assume the response came with HTTP 200 - the callback is called and startSomething() is executed.

After roughly 1500ms first showcase call fires, something went wrong on steam side and response arrives with HTTP 418 - callback is called again and exit() is executed, which happens to terminate the execution - even though startSomething() already started doing some work.

Would you expect this to happen looking on the code alone? Neither would I. Let's continue and assume we do not terminate the program, just log the error, what's gonna happen then? What result am I supposed to expect? Is there none, one or two showcases on the profile? What items are in them, can I repeat calls that have failed?

Answering the questions:

Remaining 5 showcase calls fire within 7500ms, one of them silently fails, other 4 passes.

There are 2 showcases on my profile

There is total of 4 items in my showcases, 1 in trade showcase and 3 in items showcase

I'm left with confused - my program just logged Profile set up, how could it fail over a second later?

I can only repeat blindly, there's no feedback which request failed (or even how many).

There could be even worse scenario, when one sets enough showcase items for the error to be called many seconds later - let's say a minute. Would you, as an use, expect an error to come a minute after you called a function, which callback already fired without an error long time ago? Would you even expect this function to still process after firing a callback?

I'm only pointing out possible issues, as stated in other comment, the code should already be passable.

Currently a way which I could imagine to know which specific request failed, would be adding an array types and the number calls to errorcontrol and everytime before the request is sent to "ajaxsetshowcaseconfig" push the current type of showcase and the slot into the array. Then each time the callback of it is called, calls of the errorcontrol object is incremented, then if an error happens, use calls as index in types to get the showcase type and slot. But even if you have that information, how would you tell the end user what happened when if you don't want to call the callback multiple times? The callback will always fire once from the generic information, and then fire n times again (if you throw an error every time a request fails), even if you would give back an object like {error:true, type: "trade", slot: 5} the only change is that the user now would know it happened in trade, slot 5. But the base of the problem is the same. The user has to assume that the callback is called multiple times. The only improvement would be, that he now could check for slot and showcase too, but he would have to use a different way of handling the response. (Before he could check for error and if the message contains "updating specific showcases")

Thanks for acknowledging the problem. As you noticed, it is hard to do it like that - I have a suggestion you may want to check. How about removing showcase code from editProfile method, and instead adding editShowcaseItem method, which will accept options object. The showcase edit requests doesn't seem to be related with original editProfile call in any way.

The usage may look like this:

await community.editProfile({ showcases: [{ type: 'trade', notes: 'Some note' }] }); await Promise.all(showcaseItems.map((item, index) => { return community.editShowcaseItem('trade', item, index); });

For sake of the example I'm calling all items at once and assuming everything return Promise (2018, eghm).

There's also another possible solution, if you really want to stick to single method doing all HTTP calls:

community.editProfile(settings, callback, showcaseItemCallback)

But if that's your choice, not only you'll need much better error control (stop following calls if the user is not logged in for example), a way to provide custom delays between calls, good faallback delay strategy if manual configuration not supplied, and probably some way for the user to stop them manually too. As you already noticed, that's a lot of problems 🙂

PS. Separating this logic a bit and leaving as little changes in existing code should make it easier to merge, as it'll be treated more as a new feature than breaking change (which it is right now).

@Aareksio I implemented the what you wanted, I think. It now can take either a callback or just use the Promise syntax. But I had problems when testing it, so I want to ask about it. When executing it in a normal matter, there is no problem. But when I submitted wrong values to trigger the Errors to see if both callback and Promises work and having a Debugger attached (in this case Visual Studio Code, set to break on an uncaught exception) the reject() always triggered an exception, which was also caught by catch(), but if I disabled to break on uncaught exceptions the catch phrase of the Promise caught the error without anything showing up, do you have any idea why this is happening? Is this a bug of the debugger, or has this code some mistakes.

@NetroScript I don't think adding optional promises here is a good solution, use the callbacks, especially if you're more comfortable with them. Everyone who wants promises will promisify the callback functions on their own - don't break the consistency of the whole library. And yes, it seems to be caused by debugger.

Now just clean up the code (remove unused stuff - numofrequests, error control - make sure everything is formatted properly too) and it'll be probably good.

PS. Default replacement for var should be const not let, it's a common misconception and I'd recommend to read up about it :)

Aareksio · 2018-07-22T18:14:52Z

I've listed a few instances for each class of problem

Inconsistency

obj.value and obj['value']
nocasevar, sneak_case_var, camelCaseVar
const obj = { 'key': 'value' } and const obj = { key: 'value' }
whitechars

Rash design

problem with callback being fired more than once
lack of proper error handling
jquery-prop (fixed)
config object for code after the switch instead of separate method

Misuse of language or rather poor use of language features

non descriptive for loops
config object instead of separate method mentioned above
callback problem, also mentioned above
manual index of Array.forEach (fixed)

As I stated in previous comment - I want to make it clear - it's probably already passable as for quality needs of this repo, although described above traits are still visible. If you consider my feedback meaningful, you can continue to improve, otherwise it is already good enough as it is.

Now ignore additional showcases when a specific number of showcases was already set. + Additional whitespace + some `consistency`

andrewda

Lots of inconsistent newlines and whitespace, too, that should be cleaned up.

andrewda · 2018-07-22T18:53:06Z

components/profile.js

+
+					for (var type in settings[i]) {
+
+						if(maxshowcases > 0){


Space before ( and after ).

andrewda · 2018-07-22T18:59:20Z

components/profile.js

+
+					for (var type in settings[i]) {
+
+						if(maxshowcases > 0){


While not super important here, in cases like this it's good practice to break out of your for loop after this condition, i.e.

if (maxshowcases > 0) { // ... } else { break; }

This way, you're not iterating through more times when you know you're not going to use the values.

Taking a moment on this - it may be good to invert such statements:

if (maxshowcases === 0) { break; } // ...

Agreed 👍

Though actually, I guess a more readable approach would be to define a variable currentShowcase and increment it every loop, making sure it's less than maxShowcases. It doesn't make logical sense to be manipulating the value of maxShowcases, whose name suggests it should hold the value of the absolute maximum number of showcases. For the current method, a better name for that variable would be remainingShowcases.

andrewda · 2018-07-22T19:00:27Z

components/profile.js

+									values["profile_showcase[8]"] = 8;
+
+									if (settings[i][type].hasOwnProperty("values")){
+										if (settings[i][type]["values"].hasOwnProperty("title")){


Space after ) (same for 158)

andrewda · 2018-07-22T19:01:04Z

components/profile.js

+											for (var n = 0; n < 6; n++) {
+												for (var t in type){
+													if (settings[i][type]["values"]["badges"][n] != undefined) {
+														if (settings[i][type]["values"]["badges"][n].hasOwnProperty(types[t])){


Space after ) (same for 229)

andrewda · 2018-07-22T19:05:47Z

components/profile.js

+									}
+									break;
+
+								case 'guide':


A lot of these cases contain very similar code. It's hard for me to believe this can't be done in a more efficient way that would be more easily transferable to more showcases as Valve adds them.

I could imagine something considering the possible combinations are (if I am correct)

title + notes

title

array of publishedfileids

style and array of badges

appid and publishedfileid

single value
and allowing an additional array with single item requests

But assume the readability of this would be terrible compared to the switch case

There's always a "proper" way to do something, and I think doing it in a way which allows more showcases to be easily added on is a must. I guess the current method isn't too bad, especially after a little cleanup.

In the ideal world, I'd say there should be a Showcases class containing the data that every showcase might want, such as item, title, badge, etc. Then the code here would be able to get passed a specific showcase class from the user and add that to their profile. Maybe this is a little too ambitious for something that probably won't be used too often, but it feels like there's still some work that could be done to make this all more friendly.

andrewda · 2018-07-22T19:06:17Z

components/profile.js

+									}
+									break;
+
+								case 'ownguides':


Why are there two ownguides cases?

My fault, the lower is ownworkshop

andrewda · 2018-07-22T19:09:57Z

components/profile.js

+
+									if (settings[i][type].hasOwnProperty("values")) {
+										for (var n = 0; n < 4; n++) {
+											if (settings[i][type]["values"][n] != undefined) {


This is not the proper way to check if something is undefined in JavaScript. Otherwise you risk an error. Use:

if (typeof foo === 'undefined')

https://stackoverflow.com/a/4726198/5737136

The current way wouldn't cause errors as far as I see it and also allows f.e. null instead of undefined too. Even when using variable !== undefined the variable will always be undefined or defined, because it is a property of an object. Or can you give a example where this specific code would throw an error?

You're right, this shouldn't cause any errors as-is. I think convention is to usually use typeof, though, but it's fine as it is now.

Additional whitespace changes

…nstead ediShowcaseItem() takes 4 parameters, the showcase, the slot, the item and a possible callback. If a callback is submitted the callback is called, if not a Promise is returned. (Promise is returned in all cases, but only resolved or rejected if there is no callback)

Additionally fixed small mistake (`var t in type` instead of `var t in types`)

Aareksio · 2018-07-25T14:03:20Z

components/profile.js

+
+	if(!allowedoptions.hasOwnProperty(showcase)){
+		const err = new Error("The submitted showcase type has no editable items.");
+		return callback ? callback(err) : undefined;


Using callback here instead of throw is a little counterintuitive for me, but I don't think that's enough to justify change request 🙂.

return undefined; looks really weird to me. I'd just use an if-statement here.

Aareksio · 2018-07-25T14:06:26Z

components/profile.js

+	const requestdata = item;
+	requestdata["slot"] = slot - 1;
+	requestdata["customization_type"] = allowedoptions[showcase]["type"];
+	requestdata["sessionid"] = self.getSessionID();


Objects are passed by reference, not by value. Check this out (minimal reproduction of the code above):

const item = { id: 1 }; const requestData = item; requestData .slot = 1; console.log(item, requestData ); // { id: 1, slot: 1 }, { id: 1, slot: 1 }

Solution for this may be ES6 spread and rest

const item = { id: 1 }; const requestData = { ...item, slot: 1 }; console.log(item, requestData); // { id: 1 }, { id: 1, slot: 1 }

Aareksio

Some formatting issues here and there, we could argue about code style, variable names, other preferences... But generally PR seems alright.

I'm probably a little biased after reading through it quite a few times and tracking the changes, but the code feels okay as for almost stand-alone feature it became.

This PR (finally) doesn't introduce any changes in current behavior, so it could be safely merged.

Aareksio · 2018-07-25T14:10:51Z

components/profile.js

@@ -32,6 +32,51 @@ SteamCommunity.prototype.setupProfile = function(callback) {
 	});
 };

+SteamCommunity.prototype.editShowcaseItem = function(showcase, slot, item, callback) {
+	const self = this;


I don't think there's a need for that

…ing reference Additionally changing spacing around `if` clauses. `Object.assign()` may not be a deep clone of the item object. But because only new properties are added directly to requestdata, the original item object is not modified. (Only if item had a property like `item.prop = {"test": true}` and you would change `requestdata.prop.test = false` the property of item would be changed too, but if you would change f.e. `requestdata.prop = "test"` the item object would be unmodified, so I would say `Object.assign()` is okay for this case).

Revadike · 2021-06-14T23:51:39Z

@DoctorMcKay why did this get closed? Was it not proper enough?

DoctorMcKay · 2021-06-16T09:45:00Z

@DoctorMcKay why did this get closed? Was it not proper enough?

@Revadike It looked to me like some of my comments never got addressed, but maybe GitHub just wasn't showing me changes so obviously. I'll take another look at this.

Revadike · 2021-06-16T11:51:23Z

Sure, give it another review, and perhaps I could fork it and address your feedback, if needed.

NetroScript · 2021-06-17T14:04:36Z

As the original Author I want to add, this code wouldn't work anymore (and I also completely forgot this pull was still open because it was inactive for so long).

One of the steam updates allowing you to buy showcases with points and having multiple of the same kind changed the way they work. Additionally the new steam edit UI for the profile changed the endpoint from edit to edit/showcases. The needed changes for a basic working version are minor, but to restructure it in a better way, and to allowing multiple showcases of the same type would require a bit more work.

Just as an example, the code for my bot to change the profile showcase looks like this:

                    case "infobox":
                        values["profile_showcase[8]"] = 8;
                        values["profile_showcase_purchaseid[8]"] = 0;

                        if (settings[type].hasOwnProperty("values")) {
                            if (settings[type]["values"].hasOwnProperty("title")) {
                                values["rgShowcaseConfig[8_0][0][title]"] = settings[type]["values"]["title"];
                            }
                            if (settings[type]["values"].hasOwnProperty("notes")) {
                                values["rgShowcaseConfig[8_0][0][notes]"] = settings[type]["values"]["notes"];
                            }
                        }

                        break;

instead of this (which is in this PR):

							case 'infobox':
								values["profile_showcase[8]"] = 8;

								if (settings[i][type].hasOwnProperty("values")) {
									if (settings[i][type]["values"].hasOwnProperty("title")) {
										values["rgShowcaseConfig[8][0][title]"] = settings[i][type]["values"]["title"];
									}
									if (settings[i][type]["values"].hasOwnProperty("notes")) {
										values["rgShowcaseConfig[8][0][notes]"] = settings[i][type]["values"]["notes"];
									}
								}

								break;

And there are also some small other things which changed (like the new purchaseid parameter which is included now in the requests).

But because my needs are limited for my own bot, the testing I did was only the trade and infobox showcase. But it should be pretty safe to assume that the others still work the same way.

NetroScript added 3 commits July 20, 2018 01:38

Aareksio suggested changes Jul 20, 2018

View reviewed changes

NetroScript added 4 commits July 20, 2018 23:04

Removed dependency of param

0f9ae26

Ensures the array index for the submitted settings.

de4c377

Now the array index for name for example will always be 8.

Improved readability of showcaseconfig

a8d9f16

Replaced t with type and added delay to the ajaxsetshowcaseconfig r…

a9264f2

…equests The delay is 1.5 seconds

Aareksio suggested changes Jul 22, 2018

View reviewed changes

NetroScript added 5 commits July 22, 2018 14:06

Fixing indention + removing unnecessary iterator variable

8c61d78

Improved timeout for additional changes to showcase config items.

cba563b

Now it can't happen, that f.e. games and items are changed at the same time leading to simultaneous requests.

Added missing curly braces

1b6364e

Aareksio suggested changes Jul 22, 2018

View reviewed changes

Check for the maximum number of possible showcases

bc00349

Now ignore additional showcases when a specific number of showcases was already set. + Additional whitespace + some `consistency`

andrewda suggested changes Jul 22, 2018

View reviewed changes

NetroScript added 4 commits July 22, 2018 21:32

fixed ownworkshop

1773948

Changed name to remainingshowcases + break the loop at that point

1073e70

Additional whitespace changes

Removed obsolent variables, changed editShowcaseItem to callback only

5e31a54

Additionally fixed small mistake (`var t in type` instead of `var t in types`)

Aareksio reviewed Jul 25, 2018

View reviewed changes

Aareksio approved these changes Jul 25, 2018

View reviewed changes

DoctorMcKay closed this Nov 5, 2020

DoctorMcKay reopened this Jun 16, 2021


		}

		self._myProfile("ajaxsetshowcaseconfig", requestdata, function (err, response, body) {

Added the possibility to edit showcases #207

Are you sure you want to change the base?

Added the possibility to edit showcases #207

Conversation

NetroScript commented Jul 20, 2018 • edited Loading

Aareksio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio commented Jul 20, 2018

DoctorMcKay commented Jul 20, 2018

Aareksio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio commented Jul 22, 2018

NetroScript commented Jul 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewda Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio commented Jul 22, 2018 • edited Loading

andrewda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aareksio Jul 22, 2018 • edited Loading

NetroScript commented Jul 20, 2018 •

edited

Loading

Aareksio Jul 22, 2018 •

edited

Loading

Aareksio left a comment •

edited

Loading

Aareksio Jul 22, 2018 •

edited

Loading

andrewda Jul 22, 2018 •

edited

Loading

Aareksio Jul 23, 2018 •

edited

Loading

Aareksio Jul 24, 2018 •

edited

Loading

Aareksio commented Jul 22, 2018 •

edited

Loading

Aareksio Jul 22, 2018 •

edited

Loading