PADV-973 feat: add instructor modal #40

AuraAlba · 2024-02-21T18:03:51Z

Ticket

https://agile-jira.pearson.com/browse/PADV-973

Description

Add instructor modal

Changes made

Update add instructor modal
Remove classes fetch used in the past component
Update tests

Screenshots

How to test

Clone the Institution Portal MFE
Run npm install.
Run npm run start
Go to http://localhost:1980/instructors

anfbermudezme · 2024-02-21T21:05:49Z

src/features/Instructors/AddInstructors/_test_/index.test.jsx

+describe('Add instructor modal', () => {
+  test('Render add insctructor modal', async () => {
+    const { getByText, getByPlaceholderText } = renderWithProviders(
+      <AddInstructors isOpen close={() => { }} />,


Suggested change

<AddInstructors isOpen close={() => { }} />,

<AddInstructors isOpen close={() => {}} />,

anfbermudezme · 2024-02-21T21:11:30Z

src/features/Instructors/AddInstructors/index.jsx

-        setIsNoUser(false);
-      }
+      dispatch(addInstructor(selectedInstitution.id, formJson.instructorEmail));
+      close();


We won't add confirmations at the end? @AuraAlba

This is not part of the scope but I added the success message

anfbermudezme · 2024-02-21T21:12:17Z

src/features/Instructors/AddInstructors/index.jsx

+          </FormGroup>
+          <div className="d-flex justify-content-end">
+            <ModalCloseButton className="btntpz btn-text btn-tertiary">Cancel</ModalCloseButton>
+            <Button type="submit">Send invite</Button>


Suggested change

<Button type="submit">Send invite</Button>

<Button type="submit">Send invitation</Button>

This is how is in the figma, should I change it?

anfbermudezme · 2024-02-21T21:13:30Z

src/features/Instructors/data/_test_/api.test.js

+
+    expect(httpClientMock.post).toHaveBeenCalledTimes(1);
+    expect(httpClientMock.post).toHaveBeenCalledWith(
+      'http://localhost:18000/pearson_course_operation/api/v2/instructors/[email protected]&institution_id=1',


Line too long. @AuraAlba

anfbermudezme · 2024-02-21T21:13:50Z

src/features/Instructors/data/_test_/api.test.js

@@ -1,5 +1,5 @@
 import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
-import { getInstructorData, handleInstructorsEnrollment } from 'features/Instructors/data/api';
+import { getInstructorData, handleInstructorsEnrollment, handleNewInstructor } from 'features/Instructors/data/api';


Line too long. @AuraAlba

01001110J · 2024-02-21T20:56:15Z

src/features/Instructors/AddInstructors/index.jsx

+          </FormGroup>
+          <div className="d-flex justify-content-end">
+            <ModalCloseButton className="btntpz btn-text btn-tertiary">Cancel</ModalCloseButton>
+            <Button type="submit">Send invite</Button>


I'm unsure if it would fit within the scope of this issue, but currently, the button remains enabled even if the required input field is empty. While I see that the input field has the 'required' attribute, I'm concerned that users could potentially bypass this by manipulating the HTML using browser dev tools. To strengthen the feature, would it be feasible to add client-side validation to ensure that the input field is filled before sending any requests? And of course the back can check if the email is being provided, if not throw and error, but my point of all of this is avoid just spamming the "send" button with a bunch of request.

What are your thoughts on this?

I don't have problem with this, is not a complex implementation. I added

01001110J · 2024-02-21T21:06:26Z

src/features/Instructors/data/slice.js

+    addInstructorRequest: (state) => {
+      state.addInstructor.status = RequestStatus.LOADING;
+    },
+    addInstructorSuccess: (state, { payload }) => {
+      state.addInstructor.status = RequestStatus.SUCCESS;
+      state.addInstructor.data = payload;
+    },
+    addInstructorFailed: (state) => {
+      state.addInstructor.status = RequestStatus.ERROR;
+    },
  },


I'm not sure about this names, there aren't clear, maybe:

Suggested change

addInstructorRequest: (state) => {

state.addInstructor.status = RequestStatus.LOADING;

},

addInstructorSuccess: (state, { payload }) => {

state.addInstructor.status = RequestStatus.SUCCESS;

state.addInstructor.data = payload;

},

addInstructorFailed: (state) => {

state.addInstructor.status = RequestStatus.ERROR;

},

},

setInstructorLoading: (state) => {

state.addInstructor.status = RequestStatus.LOADING;

},

setInstructorSuccessRequest: (state, { payload }) => {

state.addInstructor.status = RequestStatus.SUCCESS;

state.addInstructor.data = payload;

},

setInstructorErrorRequest: (state) => {

state.addInstructor.status = RequestStatus.ERROR;

},

},

Usually when we defined a reducer is not a good practice write the name as a setter, I will find a name more descriptive

I do not modify this, for me it is quite clear to describe the actions that the reducer performs and adding the request word does not contribute to this description

01001110J · 2024-02-21T21:09:46Z

src/features/Instructors/data/thunks.js

@@ -44,37 +44,37 @@ function fetchCoursesData(id) {
  };
 }

-function fetchClassesData() {
+function assignInstructors(data, classId, institutionId) {


I think it could be a good addition to use jsdocs for documentation purposes:

Suggested change

function assignInstructors(data, classId, institutionId) {

/**

* Assign instructors to a class.

*

* @param {Object} data - The data containing information about the instructors to be assigned.

* @param {string} classId - The ID of the class to which the instructors will be assigned.

* @param {string} institutionId - The ID of the institution associated with the class.

* @returns {Promise<void>} - A promise that resolves after dispatching appropriate actions.

*/

function assignInstructors(data, classId, institutionId) {

01001110J · 2024-02-21T21:10:00Z

src/features/Instructors/data/thunks.js

    }
  };
 }

-function assignInstructors(data, classId, institutionId) {
+function addInstructor(institutionId, instructorEmail) {


Same comment about docs

01001110J · 2024-02-21T21:12:07Z

src/features/Instructors/AddInstructors/index.jsx

-  const stateInstructors = useSelector((state) => state.instructors.classes);
+import { addInstructor } from 'features/Instructors/data/thunks';
+
+const AddInstructors = ({ isOpen, close }) => {


I think close is not clear enough, maybe handleOnClose or onClose could be more descriptive.

01001110J · 2024-02-21T21:15:35Z

src/features/Instructors/InstructorsPage/index.jsx

@@ -16,6 +17,7 @@ const InstructorsPage = () => {
  const selectedInstitution = useSelector((state) => state.main.selectedInstitution);
  const dispatch = useDispatch();
  const [currentPage, setCurrentPage] = useState(initialPage);
+  const [isOpen, open, close] = useToggle(false);


This names does not explain itself what's supposed to should do, maybe:

Suggested change

const [isOpen, open, close] = useToggle(false);

const [isOpen, openMolda, closeModal] = useToggle(false);

anfbermudezme self-requested a review February 21, 2024 20:56

AuraAlba requested a review from 01001110J February 21, 2024 20:57

anfbermudezme requested changes Feb 21, 2024

View reviewed changes

01001110J reviewed Feb 21, 2024

View reviewed changes

feat: add instructor modal

51b12e6

AuraAlba force-pushed the vue/PADV-973 branch from 24bf9a6 to 51b12e6 Compare February 22, 2024 16:48

anfbermudezme approved these changes Feb 22, 2024

View reviewed changes

01001110J approved these changes Feb 22, 2024

View reviewed changes

AuraAlba merged commit 389a2ed into master Feb 22, 2024
2 checks passed

AuraAlba deleted the vue/PADV-973 branch February 22, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PADV-973 feat: add instructor modal #40

PADV-973 feat: add instructor modal #40

AuraAlba commented Feb 21, 2024

anfbermudezme Feb 21, 2024

AuraAlba Feb 22, 2024

anfbermudezme Feb 21, 2024

AuraAlba Feb 22, 2024

anfbermudezme Feb 21, 2024

AuraAlba Feb 22, 2024

anfbermudezme Feb 21, 2024

AuraAlba Feb 22, 2024

anfbermudezme Feb 21, 2024

AuraAlba Feb 22, 2024

01001110J Feb 21, 2024

AuraAlba Feb 22, 2024

01001110J Feb 21, 2024

AuraAlba Feb 22, 2024

AuraAlba Feb 22, 2024

01001110J Feb 21, 2024

AuraAlba Feb 22, 2024

01001110J Feb 21, 2024

AuraAlba Feb 22, 2024

01001110J Feb 21, 2024

AuraAlba Feb 22, 2024

01001110J Feb 21, 2024

AuraAlba Feb 22, 2024

	<AddInstructors isOpen close={() => { }} />,
	<AddInstructors isOpen close={() => {}} />,

	<Button type="submit">Send invite</Button>
	<Button type="submit">Send invitation</Button>

-function assignInstructors(data, classId, institutionId) {
+/**
+ * Assign instructors to a class.
+ *
+ * @param {Object} data - The data containing information about the instructors to be assigned.
+ * @param {string} classId - The ID of the class to which the instructors will be assigned.
+ * @param {string} institutionId - The ID of the institution associated with the class.
+ * @returns {Promise<void>} - A promise that resolves after dispatching appropriate actions.
+ */
+function assignInstructors(data, classId, institutionId) {

	const [isOpen, open, close] = useToggle(false);
	const [isOpen, openMolda, closeModal] = useToggle(false);

PADV-973 feat: add instructor modal #40

PADV-973 feat: add instructor modal #40

Conversation

AuraAlba commented Feb 21, 2024

Ticket

Description

Changes made

Screenshots

How to test

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