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

Add route to get brew styling #3075

Merged
merged 22 commits into from
Aug 27, 2024

Conversation

G-Ambatte
Copy link
Collaborator

This PR resolves #1097.

This PR adds the /css route, which returns the stylesheet for a given brew share ID.

@G-Ambatte G-Ambatte self-assigned this Oct 1, 2023
@G-Ambatte G-Ambatte added the P2 - minor feature or not urgent Minor bugs or less-popular features label Oct 1, 2023
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 October 3, 2023 15:30 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 January 17, 2024 17:22 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 January 18, 2024 20:04 Inactive
@Gazook89 Gazook89 added the 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed label Feb 17, 2024
@G-Ambatte G-Ambatte added blocked Waiting on a dependency, other feature, etc., first and removed P2 - minor feature or not urgent Minor bugs or less-popular features 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Feb 24, 2024
@5e-Cleric
Copy link
Member

What is this branch blocked by?

@calculuschild
Copy link
Member

The User Themes PR #3321 implemented a recursive form of this (retrieve CSS, and also @include CSS from parent theme, which calls it's parent theme, etc.), but that has since been overridden with a dedicated /api/themes endpoint that gets both CSS and any associated snippets (with potential for retrieving other theme-related properties) in a bundle, including from all theme parents.

However, it has been requested that we still implement a CSS-only endpoint but just for individual brews, not recursively, which leads us back here.

I'm going to copy here the recursive implementation from #3321 along with its test cases, as I think there is still some bits that might be helpful in finishing out this PR:

CSS Retrieval
	//Return CSS for a brew theme, with @include endpoint for its parent theme if any
	getBrewThemeCSS : async (req, res)=>{
		const brew = req.brew;
		splitTextStyleAndMetadata(brew);
		res.setHeader('Content-Type', 'text/css');
		let rendererPath = '';
		if(isStaticTheme(req.brew.renderer, req.brew.theme)) //Check if parent is staticBrew
			rendererPath = `${_.upperFirst(req.brew.renderer)}/`;

		const parentThemeImport = `@import url(\"/css/${rendererPath}${req.brew.theme}\");\n\n`;
		const themeLocationComment = `/* From Brew: ${req.protocol}://${req.get('host')}/share/${req.brew.shareId} */\n\n`;
		return res.status(200).send(`${parentThemeImport}${themeLocationComment}${req.brew.style}`);
	},
	//Return CSS for a static theme, with @include endpoint for its parent theme if any
	getStaticThemeCSS : async(req, res)=>{
		if(!isStaticTheme(req.params.renderer, req.params.id))
			res.status(404).send(`Invalid Theme - Renderer: ${req.params.renderer}, Name: ${req.params.id}`);
		else {
			res.setHeader('Content-Type', 'text/css');
			res.setHeader('Cache-Control', 'public, max-age: 43200, must-revalidate');
			const themeParent = Themes[req.params.renderer][req.params.id].baseTheme;
			const parentThemeImport = themeParent ? `@import url(\"/css/${req.params.renderer}/${themeParent}\");\n/* Static Theme ${Themes[req.params.renderer][themeParent].name} */\n` : '';
			return res.status(200).send(`${parentThemeImport}@import url(\"/themes/${req.params.renderer}/${req.params.id}/style.css\");\n/* Static Theme ${Themes[req.params.renderer][req.params.id].name} */\n`);
		}
	},
Testing
	describe('getBrewThemeWithStaticParent', ()=>{
		it('should collect parent theme and brew style - returning as css with static parent imported.', async ()=>{
			const toBrewPromise = (brew)=>new Promise((res)=>res({ toObject: ()=>brew }));
			model.get = jest.fn(()=>toBrewPromise({ title: 'test brew', renderer: 'V3', theme: '5eDMG', shareId: 'iAmAUserTheme', style: 'I Have a style!' }));
			const fn = api.getBrew('share', true);
			const req = { brew: {}, get: ()=>{return 'localhost';}, protocol: 'https' };
			const next = jest.fn();
			await fn(req, null, next);

			api.getBrewThemeCSS(req, res);
			const sent = res.send.mock.calls[0][0];
			expect(sent).toBe(`@import url("/css/V3/5eDMG");\n\n/* From Brew: https://localhost/share/iAmAUserTheme */\n\nI Have a style!`);
			expect(res.status).toHaveBeenCalledWith(200);
		});
	});

	describe('getBrewThemeWithUserParent', ()=>{
		it('should collect parent theme and brew style - returning as css with user-theme parent imported.', async ()=>{
			const toBrewPromise = (brew)=>new Promise((res)=>res({ toObject: ()=>brew }));
			model.get = jest.fn(()=>toBrewPromise({ title: 'test brew', renderer: 'V3',  shareId: 'iAmAUserTheme', theme: 'IamATheme', style: 'I Have a style!' }));
			const fn = api.getBrew('share', true);
			const req = { brew: {}, get: ()=>{return 'localhost';}, protocol: 'https' };
			const next = jest.fn();
			await fn(req, null, next);

			api.getBrewThemeCSS(req, res);
			const sent = res.send.mock.calls[0][0];
			expect(sent).toBe(`@import url("/css/IamATheme");\n\n/* From Brew: https://localhost/share/iAmAUserTheme */\n\nI Have a style!`);
			expect(res.status).toHaveBeenCalledWith(200);
		});
	});

	describe('getStaticThemeCSS', ()=>{
		it('should return an import of the theme including a parent.', async ()=>{
			const req = {
				params : {
					renderer : 'V3',
					id       : '5eDMG'
				}
			};
			api.getStaticThemeCSS(req, res);
			const sent = res.send.mock.calls[0][0];
			expect(sent).toBe('@import url("/css/V3/5ePHB");\n/* Static Theme 5e PHB */\n@import url("/themes/V3/5eDMG/style.css");\n/* Static Theme 5e DMG */\n');
			expect(res.status).toHaveBeenCalledWith(200);
		});
		it('should fail for an invalid static theme.', async()=>{
			const req = {
				params : {
					renderer : 'V3',
					id       : '5eDMGGGG'
				}
			};
			api.getStaticThemeCSS(req, res);
			const sent = res.send.mock.calls[0][0];
			expect(sent).toBe('Invalid Theme - Renderer: V3, Name: 5eDMGGGG');
			expect(res.status).toHaveBeenCalledWith(404);
		});
	});

calculuschild added a commit to dbolack-ab/homebrewery that referenced this pull request Jul 28, 2024
Now that we have a dedicated /theme/ route for the recursive theming, the CSS endpoint can be simpler for only getting the `style` of a single brew. naturalcrit#3075 already has this simpler version, but no testing, so I have copied this into a comment there for implementation when it is ready.
@dbolack-ab
Copy link
Collaborator

You can functionally get the present theme with the bundle endpoint as they are ordered.

@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 1, 2024 18:28 Inactive
Copy link
Member

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

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

Seems functional.

Can we just add two test cases? One for when the :id exists, and one where it doesn't? I.e. make sure we handle the error OK.

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R_ - Not ready for review 🕑 labels Aug 1, 2024
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 9, 2024 03:30 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 13, 2024 10:05 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 13, 2024 10:31 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 16, 2024 22:25 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 16, 2024 22:27 Inactive
Copy link
Collaborator

@dbolack-ab dbolack-ab left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@5e-Cleric 5e-Cleric left a comment

Choose a reason for hiding this comment

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

Looks good to me too

@5e-Cleric 5e-Cleric added 🔍 R4 - Fixed! Awaiting re-review⭐ PR review comments addressed and removed 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging labels Aug 22, 2024
@5e-Cleric
Copy link
Member

@G-Ambatte is there something left to do here?

@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 23, 2024 21:16 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 26, 2024 23:58 Inactive
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3075 August 27, 2024 00:02 Inactive
@calculuschild
Copy link
Member

I just added a couple more checks to the "successful" test, to test if we are sending a 200 status and that the content of the response is just the plain CSS content. Going to merge now. Thanks all!

@calculuschild calculuschild merged commit 183687d into naturalcrit:master Aug 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

API or route to produce CSS resource
5 participants