feat: Automatic publication of admins #40

redwan-faris · 2024-06-10T14:20:33Z

This is a:

✨ New feature - new behaviour has been implemented
🐛 Bug fix - existing behaviour has been made to behave
♻️ Refactor - the behaviour has not changed, just the implementation
✅ Test backfill - tests for existing behaviour were added but the behaviour itself hasn't changed
⚙️ Chore - maintenance task, behaviour and implementation haven't changed

Description

Purpose - the admin can add the suggestion and its will be published automatically without going to draft

How to check -
login as admin and then add the suggestion and check it in the publication directly

Links

Author checklist

I have written a title that reflects the relevant ticket
I have written a description that says what the PR does and how to validate it
I have linked to the project board ticket (and any related PRs/issues) in the Links section
I have added a link to this PR to the ticket
I have made the PR to main from a branch named <category>/<name>, e.g. feature/edit-spaceships or bugfix/restore-oxygen
I have manually tested that the app still works correctly
I have requested reviewers here and in my team chat channel

I have spoken with my PM or TL about any parts of this task that may have become out-of-scope, or any additional improvements that I now realise may benefit my project
I have added tests, or new tests were not required
I have updated any documentation (e.g. diagrams, schemas), or documentation updates were not required

textbook · 2024-06-10T15:47:58Z

Please do review the PR template in the description and fill it out accordingly.

redwan-faris · 2024-06-10T19:18:59Z

Please do review the PR template in the description and fill it out accordingly.
I did that
Thank you

textbook

The basic functionality (when an admin posts a resource, publish it immediately) works OK. But looking at how that's done, it's clear the suggested flow (.github/CONTRIBUTING.md) wasn't followed. The checklist in the PR template is intended as a reminder to actually do those things - link the issue, write the tests, update the docs, ...

Functionality

✅ When I suggest a resource as a non-admin, it is created as a draft. When I do it as an admin, it's immediately published.

💡 UI is misleading when an admin is using the Suggest form - it still tells them their submission will be reviewed by an admin:

Implementation

❓ Are we doing too much in the controller - maybe the service should be deciding whether to create a draft or published resource (i.e. receiving both information about the resource and the person creating it, something like service.create(resource, user)) as that's part of our business logic not specific to to the transport layer.

❌ No E2E test

❌ No API integration test

❓ Do the docs need updating to reflect the changed behaviour of the endpoint?

Key

✅ Nothing to change, just commenting
💡 Possible change, or suggestion to think about in the future
❓ Discussion required before approval
❌ Change required before approval

textbook · 2024-06-13T09:16:58Z

server/resources/resourcesController.js

+				let isDraft = true;
+
+				if (req.superuser || req.user?.is_admin) {
+					isDraft = false;
+				}


💡 Using the optional chaining here is unnecessary - if req.user was null or undefined, this would already have crashed on line 48 (you could also have accessed is_admin there, to keep all references to the req.user in one place).

💡 I'd always try to avoid using let for something like this, we can determine the value straight away and make it const:

Suggested change

let isDraft = true;

if (req.superuser || req.user?.is_admin) {

isDraft = false;

}

const isDraft = (req.superuser || req.user?.is_admin) ? false : true;

But then having a conditional expression whose values are true and false seems a bit weird, just use the value directly (ref: https://en.wikipedia.org/wiki/De_Morgan%27s_laws):

Suggested change

let isDraft = true;

if (req.superuser || req.user?.is_admin) {

isDraft = false;

}

const isDraft = !req.superuser && !req.user?.is_admin;

…on response

redwan-faris · 2024-06-16T21:50:17Z

The basic functionality (when an admin posts a resource, publish it immediately) works OK. But looking at how that's done, it's clear the suggested flow (.github/CONTRIBUTING.md) wasn't followed. The checklist in the PR template is intended as a reminder to actually do those things - link the issue, write the tests, update the docs, ...

Functionality

✅ When I suggest a resource as a non-admin, it is created as a draft. When I do it as an admin, it's immediately published.

💡 UI is misleading when an admin is using the Suggest form - it still tells them their submission will be reviewed by an admin:

Implementation

❓ Are we doing too much in the controller - maybe the service should be deciding whether to create a draft or published resource (i.e. receiving both information about the resource and the person creating it, something like service.create(resource, user)) as that's part of our business logic not specific to to the transport layer.

❌ No E2E test

❌ No API integration test

❓ Do the docs need updating to reflect the changed behaviour of the endpoint?

Key

✅ Nothing to change, just commenting

💡 Possible change, or suggestion to think about in the future

❓ Discussion required before approval

❌ Change required before approval

Thank you for your time and effort in reviewing my code. I have refactored the code in the API section to make it clearer, placing each piece of code in its appropriate file. Additionally, I have modified the responses that appear on the client side.

However, as a backend developer, I am facing problems with client-side testing.

Could you help me with changing the authentication for the user? The test is failing because it signs in using the admin account by default, but I want to control the sign-in process so I can create tests for both types of roles in the system.

Thanks.

textbook · 2024-06-17T15:39:47Z

Could you help me with changing the authentication for the user? The test is failing because it signs in using the admin account by default, but I want to control the sign-in process so I can create tests for both types of roles in the system.

What test is failing, and what exactly is it saying? You can push work in progress to the branch, then it'll run in the pipeline and we can all see it.

Across the various existing E2E cases there are examples of signing in using both admin and non-admin users, so testing both roles should be resonably straightforward.

redwan-faris · 2024-06-18T22:23:19Z

Could you help me with changing the authentication for the user? The test is failing because it signs in using the admin account by default, but I want to control the sign-in process so I can create tests for both types of roles in the system.

What test is failing, and what exactly is it saying? You can push work in progress to the branch, then it'll run in the pipeline and we can all see it.

Across the various existing E2E cases there are examples of signing in using both admin and non-admin users, so testing both roles should be resonably straightforward.

I apologize for the delayed response. This week, I am celebrating Eid, an important holiday in Iraq ,
e2e is ok for me and the testing is working now
I am talking about the testing in the client side
how ever I will push the code as soon as possible
Thank you

redwan-faris · 2024-06-20T19:06:32Z

Could you help me with changing the authentication for the user? The test is failing because it signs in using the admin account by default, but I want to control the sign-in process so I can create tests for both types of roles in the system.

What test is failing, and what exactly is it saying? You can push work in progress to the branch, then it'll run in the pipeline and we can all see it.
Across the various existing E2E cases there are examples of signing in using both admin and non-admin users, so testing both roles should be resonably straightforward.

I apologize for the delayed response. This week, I am celebrating Eid, an important holiday in Iraq , e2e is ok for me and the testing is working now I am talking about the testing in the client side how ever I will push the code as soon as possible Thank you

@textbook

textbook · 2024-06-21T08:58:18Z

I have seen those messages. Do you have a specific question? You can see the tests failing in the pipeline, if that's what you're asking about - they give a lot of feedback, and I don't think "failing because it signs in using the admin account by default" is a correct interpretation of what's happening - the client-side tests don't sign in at all, the network layer is mocked with MSW.

redwan-faris · 2024-06-21T13:31:43Z

I have seen those messages. Do you have a specific question? You can see the tests failing in the pipeline, if that's what you're asking about - they give a lot of feedback, and I don't think "failing because it signs in using the admin account by default" is a correct interpretation of what's happening - the client-side tests don't sign in at all, the network layer is mocked with MSW.

I think every thing related to tested is done now
Api testing
E2E testing
Client side testing

The swagger does not requeired to update I think because the only think that changes is the response of resource creation
when you are admin the draft will be false

Also is there any project linked with this repo to link the ticket?

Thank you for your time
@textbook

textbook

Thank you for the iteration, the test coverage is much improved. How did you find writing them? What about the refactoring from the controller to the service, what did you think of that?

is there any project linked with this repo to link the ticket?

You can just link the issue, #11

textbook · 2024-06-21T19:13:51Z

client/src/pages/Suggest/Suggest.test.jsx

+		if (requestBody.draft) {
+			await expect(
+				screen.findByText(/thank you for suggesting a resource/i)
+			).resolves.toHaveClass("message", "success");
+		} else {
+			await expect(
+				screen.findByText(/thank you for publishing a resource/i)
+			).resolves.toHaveClass("message", "success");
+		}


❌ You should pretty much never have conditional logic in tests. Only one of these cases is ever actually tested, there's no test where "suggesting" would be shown (which is what made them start failing in the first place), so including the other branch just confuses the issue and implies that's tested when it isn't.

❌ It's also incorrect - whether it shows "suggesting" or "publishing" isn't a matter of the request body (which never has a draft property - this should be clear from the assertions on it), it's a matter of the response body (which is an empty object in all of these tests).

ok I got that thanks

textbook · 2024-06-21T19:15:53Z

client/src/pages/Suggest/index.jsx

 			<p>
-				Please use the form below to submit a suggestion. Note that it will not
+				{isAdmin
+					? "Please use the form below to submit a resource. It's will appear immediately to the home page because you are administrator"


💡 Typos in UI text

Suggested change

? "Please use the form below to submit a resource. It's will appear immediately to the home page because you are administrator"

? "Please use the form below to submit a resource. It will appear immediately on the home page because you are administrator."

textbook · 2024-06-21T19:20:15Z

e2e/integration/suggest.test.js

+			cy.request({
+				headers: { Authorization: `Bearer ${Cypress.env("SUDO_TOKEN")}` },
+				method: "PATCH",
+				url: `/api/resources/${created.id}`,
+			});


❌ The PATCH request is to publish the resource, so that it shows up on the home page, which doesn't make sense in a test where: 1. the resource is already published (because the user is an admin); and 2. the home page isn't subsequently visited anyway.

ohhh that right :( mybe I forgot to delete it

textbook · 2024-06-21T19:22:52Z

server/resources/resources.test.js

@@ -385,4 +385,33 @@ describe("/api/resources", () => {
 			expect(published).toMatchObject({ publisher: admin.id, source: user.id });
 		});
 	});
+
+	describe("POST / - Auto publish for the admin resource", () => {


💡 This should really appear under the existing POST / group of tests; you can nest describe blocks where you really need separate contexts:

describe("/api/resources", () => { describe("POST /", () => { describe("as a user", () => { // ... }); describe("as an admin", () => { // ... }); }); });

textbook · 2024-06-21T19:25:25Z

e2e/integration/suggest.test.js

+	cy.visit("/");
+
+	cy.task("seed", { users: [admin] });
+	cy.logInAs("[email protected]");
+	cy.visit("/");


💡 It seems odd to visit the page, then update the DB, then login, then reload the home page (which you're on after logging in), maybe:

Suggested change

cy.visit("/");

cy.task("seed", { users: [admin] });

cy.logInAs("[email protected]");

cy.visit("/");

cy.task("seed", { users: [admin] });

cy.visit("/");

cy.logInAs("[email protected]");

textbook · 2024-06-21T19:27:50Z

client/src/pages/Suggest/index.jsx

@@ -12,7 +13,8 @@ export default function Suggest() {
 	const [topics, setTopics] = useState(undefined);
 	const resourceService = useService(ResourceService);
 	const topicService = useService(TopicService);
-
+	const user = usePrincipal();
+	const isAdmin = user && user.is_admin;


💡 or maybe

Suggested change

const isAdmin = user && user.is_admin;

const isAdmin = user?.is_admin;

textbook · 2024-06-21T19:28:29Z

client/src/pages/Suggest/index.jsx

+					text: resource.draft
+						? "Thank you for suggesting a resource!"
+						: "Thank you for publishing a resource!",


✅ This is neat, basing it on the response means it continues to work even if the logic around roles changes later on.

textbook · 2024-06-21T19:29:18Z

client/src/pages/Suggest/index.jsx

@@ -41,11 +45,13 @@ export default function Suggest() {

 	return (
 		<>
-			<h2>Suggest a resource</h2>
+			<h2>{isAdmin ? "Publish a resource" : "Suggest a resource"}</h2>


💡 or

Suggested change

<h2>{isAdmin ? "Publish a resource" : "Suggest a resource"}</h2>

<h2>{isAdmin ? "Publish" : "Suggest"} a resource</h2>

textbook · 2024-06-21T19:32:36Z

client/src/pages/Suggest/index.jsx

💡 Continuing from the comment on the nav, this now has lots of separate checks of the same thing, which is a bit awkward. It'd be neater to have one higher-level e.g. isAdmin ? <Publish {...} /> : <Suggest {...} />.

textbook · 2024-06-21T19:34:11Z

server/resources/resources.test.js

+			expect(body).toMatchObject({
+				title: resource.title,
+				url: resource.url,
+				draft: false,
+				id: expect.stringMatching(patterns.UUID),
+				source: id,
+				description: null,
+				accession: expect.stringMatching(patterns.DATETIME),
+			});


💡 Most of this is already asserted on in other tests, I wonder if the core logic being tested here would be clearer if the assertion was more focused:

Suggested change

expect(body).toMatchObject({

title: resource.title,

url: resource.url,

draft: false,

id: expect.stringMatching(patterns.UUID),

source: id,

description: null,

accession: expect.stringMatching(patterns.DATETIME),

});

expect(body).toHaveProperty("draft", false);

textbook · 2024-06-21T19:38:02Z

This is now complete from the application perspective.

redwan-faris · 2024-06-21T20:34:30Z

thank you for your effors and comments that I will sure work on them in the future

redwan-faris · 2024-06-21T20:42:43Z

Thank you for the iteration, the test coverage is much improved. How did you find writing them? What about the refactoring from the controller to the service, what did you think of that?

is there any project linked with this repo to link the ticket?

You can just link the issue, #11

I have reviewed the client testing and end-to-end (e2e) testing, and I found them to be very useful for completing the task. Regarding the controller logic, I realized I had misunderstood the project's code structure. I initially thought all logic should be placed in the service layer, but I noticed some validation and checks being performed in the controller as well. However, I now understand that the correct approach is indeed to place most of the business logic in the service layer.

feat: Automatic publication of admins

5a36756

kfklein15 requested review from textbook and safeamiiir June 13, 2024 09:06

textbook suggested changes Jun 13, 2024

View reviewed changes

redwan-faris added 2 commits June 16, 2024 20:25

refactor: isDraft determination logic into the service layer

ceae371

feat:Add dynamic message based on isDraft status in resource suggesti…

c2df1d6

…on response

kfklein15 requested a review from textbook June 17, 2024 15:26

redwan-faris added 2 commits June 19, 2024 01:24

fix:fix the publish error

838a661

test: add server side testing for the auto publishing

c169492

textbook mentioned this pull request Jun 21, 2024

Automatic publication of admins' resource suggestions #11

Open

redwan-faris added 4 commits June 21, 2024 15:11

test: complete e2e test for suggestion

16b94db

test: complete client side testing for puglishing resource by admin

7339a36

feat: change suggest to publish when sign in as admin

3472a9a

fix: change user.is_admin to isAdmin

e818fc7

textbook reviewed Jun 21, 2024

View reviewed changes

textbook closed this Jun 21, 2024

	? "Please use the form below to submit a resource. It's will appear immediately to the home page because you are administrator"
	? "Please use the form below to submit a resource. It will appear immediately on the home page because you are administrator."

	const isAdmin = user && user.is_admin;
	const isAdmin = user?.is_admin;

	<h2>{isAdmin ? "Publish a resource" : "Suggest a resource"}</h2>
	<h2>{isAdmin ? "Publish" : "Suggest"} a resource</h2>

Uh oh!

feat: Automatic publication of admins #40

feat: Automatic publication of admins #40

Uh oh!

Conversation

redwan-faris commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links

Author checklist

Uh oh!

textbook commented Jun 10, 2024

Uh oh!

redwan-faris commented Jun 10, 2024

Uh oh!

textbook left a comment

Choose a reason for hiding this comment

Functionality

Implementation

Key

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redwan-faris commented Jun 16, 2024

Functionality

Implementation

Key

Uh oh!

textbook commented Jun 17, 2024

Uh oh!

redwan-faris commented Jun 18, 2024

Uh oh!

redwan-faris commented Jun 20, 2024

Uh oh!

textbook commented Jun 21, 2024

Uh oh!

redwan-faris commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

textbook left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

textbook commented Jun 21, 2024

Uh oh!

redwan-faris commented Jun 21, 2024

Uh oh!

redwan-faris commented Jun 21, 2024

Uh oh!

Uh oh!

redwan-faris commented Jun 10, 2024 •

edited

Loading

redwan-faris commented Jun 21, 2024 •

edited

Loading