Skip to content

Fixes 'spo site set' command to handle site logo and thumbnail from another site. Closes #6689 #6690

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jhholm
Copy link

@jhholm jhholm commented Apr 17, 2025

Closes #6689

@Jwaegebaert
Copy link
Contributor

Thanks for jumping in with a PR to fix the bug! Looks like our coverage tests didn’t pass. We require full coverage, and it seems to have trouble with the new string you added in SpoCommand.ts: 'siteThumbnailUrl'. Could you check that out?

@jhholm
Copy link
Author

jhholm commented Apr 17, 2025

Thanks for noticing. Should be fixed now. Though I did get the same error with coverage even with this fix initially, but after the fourth time it started magically working. I think it's a user error here, cause I'm not familiar with mocha.

So the change in SpoCommand.ts was to get siteThumbnailUrl param to work like siteLogoUrl param, being automatically processed to an absolute Url. That makes the change in site-set.ts work.

@Jwaegebaert
Copy link
Contributor

Jwaegebaert commented Apr 18, 2025

Alright, good to know. Looks like updating the spec file clears the coverage, so it should be good for review. Thanks again, we'll aim to review it asap!

@waldekmastykarz waldekmastykarz self-assigned this Apr 24, 2025
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Let's add a test case that proves that this change works. I'll mark the PR as draft for now, so that we know that we should wait with merging it.

@waldekmastykarz waldekmastykarz marked this pull request as draft April 24, 2025 07:35
@jhholm
Copy link
Author

jhholm commented May 5, 2025

Is this test case something that you would want me to contribute?

I guess the following test case should cover the change

  • Test that a given relative URL is being converted correctly to an absolute URL
  • Test that an absolute URL is converted back to a relative URL correctly for the REST call

@milanholemans
Copy link
Contributor

What @waldekmastykarz means is that you should add a test to the site-set.spec.ts file that tests if the command is still working fine when specifying a site logo or thumbnail URL that is stored on another site.

@jhholm
Copy link
Author

jhholm commented May 15, 2025

Added 4 additional test cases that fail successfully on the current main branch and do not in this pull request.

@jhholm jhholm marked this pull request as ready for review May 15, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug report: Unable to set site logo or site thumbnail from another site
4 participants