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

Fix sharedfiles scraping error when Steam returns non-English page, fix rejected request detection and improve type detection #316

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

Conversation

3urobeat
Copy link
Contributor

Hey! I've gotten multiple bug reports that scraping the Posted date of a sharedfile can cause an error as Steam can apparently serve a non-English page as response to our request.
I didn't consider this to be a possibility and didn't encounter this issue while testing, even though my accounts are also not English but German.

I have added a &l=english query parameter to the request and added a fallback to the Posted readout, so that even if it should fail again, no error will be thrown and the Posted value remains null.

This fix has been tested and the users who reported it confirmed that the issue did not reoccur afterwards.

@3urobeat
Copy link
Contributor Author

This PR now also improves type detection because I don't want to get on your nerves with another PR.

Some Artworks can have only one breadcrumb (for some reason) so this change looks at the first breadcrumb as a backup if the second one is missing. I have also added an if check to hopefully suppress further errors and instead just leave type at null.

@3urobeat 3urobeat changed the title Fix sharedfiles scraping error when Steam returns non-English page Fix sharedfiles scraping error when Steam returns non-English page, fix rejected request detection and improve type detection Sep 28, 2023
@Revadike
Copy link
Contributor

You prob want to merge it into #230

@3urobeat
Copy link
Contributor Author

This PR improves an existing feature in version 3.x.x, so I chose master
DoctorMcKay merged my previous PRs into v4 as well, so I think this shouldn't be a problem(?)

@Revadike
Copy link
Contributor

Oh okay, I didn't know you were aware that v4 was being worked on

@DoctorMcKay
Copy link
Owner

Yeah, targeting master is appropriate for fixes to an existing feature, until v4 is out. I'm sorry about sorta ghosting this, been pretty busy for the past while. I'll try to deal with some open stuff, including this, later today.

Comment on lines +37 to +41
if (body.success) {
callback(null);
} else {
callback(new Error(body.error));
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this tested and confirmed working? Typically, when Steam returns a success key, it's an eresult and not a bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, Steam returns the following here: { success: false, error: 'There was a problem deleting this comment. Please try again.' }

Comment on lines +99 to +103
if (body.success) {
callback(null);
} else {
callback(new Error(body.error));
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same question as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also tested; Steam returns the following: { success: false, error: 'There was a problem posting your comment. Please try again.' }

@3urobeat 3urobeat requested a review from DoctorMcKay May 5, 2024 10:35
@3urobeat
Copy link
Contributor Author

3urobeat commented May 5, 2024

Hey, sorry for bumping, but it would be cool if you could review this PR as it fixes bugs present in the master branch.
(There are also two more of my PRs still open, but they add new features and merge into v4 instead)

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

Successfully merging this pull request may close these issues.

3 participants