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.

Sign up for GitHub

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

Jump to bottom

PADV-1007 - Filter License Pool View by Course and name #39

Merged
merged 1 commit into from
Feb 23, 2024
Merged

PADV-1007 - Filter License Pool View by Course and name #39

merged 1 commit into from
Feb 23, 2024

Conversation

01001110J
Copy link
Contributor

@01001110J 01001110J commented Feb 20, 2024 •
edited
Loading

Description

This PR adds the LicensesFilters component, allowing users to filter licenses based on courses. It integrates the selector from React Paragon and Topaz libraries for courses options and utilizes form control from Paragon for license names. This implementation aims to enhance user experience and simplify the filtering process.

This PR resolves ticket

Change log

  • Created the LicensesFilters component.
  • Fix typo in src/features/Licenses/data/_test_/redux.test.jsx.
  • The component is integrated in the view src/features/Licenses/LicensesPage/index.jsx.
  • Updated unit test.

Visual results

Before

1b

After

1
2

How to test

Clone the Institution Portal MFE

     git clone <url>

Install the packages 📦.

     npm i

Start project.

     npm start

Finally, go to http://localhost:1980/licenses and see the results.

@01001110J 01001110J self-assigned this Feb 20, 2024
@01001110J 01001110J changed the title feat: Filter by curse name and license name PADV-1007 - Filter License Pool View by Course and name Feb 20, 2024
@01001110J 01001110J marked this pull request as ready for review February 21, 2024 17:38
AuraAlba
AuraAlba requested changes Feb 21, 2024
View reviewed changes
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/index.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/index.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesPage/index.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesPage/index.jsx Show resolved Hide resolved
src/features/Licenses/LicensesFilters/index.jsx Show resolved Hide resolved
anfbermudezme
anfbermudezme requested changes Feb 21, 2024
View reviewed changes
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/__test__/index.test.jsx Show resolved Hide resolved
src/features/Licenses/data/_test_/redux.test.jsx Outdated Show resolved Hide resolved
src/features/Licenses/LicensesFilters/index.jsx Outdated
const form = e.target;
const formData = new FormData(form);
const formJson = Object.fromEntries(formData.entries());
const urlParams = Object.entries(formJson)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows me to capture the select values of the form, then dynamically build the url params to make the request.

src/features/Licenses/LicensesFilters/index.jsx Outdated
dispatch(updateFilters(formJson));
dispatch(updateCurrentPage(initialPage));
licenseData.current = licenseOptions;
dispatch(fetchLicensesData(institution.id, initialPage, urlParams));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are tou using urlParams as a parameter since it is no necessary in the thunk?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the url params is required, maybe the thunk params isn't clear, so I'll check it

Copy link
Contributor

Choose a reason for hiding this comment

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

url params name is too general and I think this url params are only for filters

Suggested change
dispatch(fetchLicensesData(institution.id, initialPage, urlParams));
dispatch(fetchLicensesData(institution.id, initialPage, urlParamsFilters));

AuraAlba
AuraAlba requested changes Feb 22, 2024
View reviewed changes
src/features/Licenses/data/thunks.js Outdated
@@ -8,11 +8,11 @@ import {
fetchLicensesDataFailed,
} from 'features/Licenses/data/slice';

function fetchLicensesData(id, currentPage) {
function fetchLicensesData(id, currentPage, urlParams) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is better to name it filtersData because is the data you are passing in this parameter, the others variables(currentPage, institutionId) are also urlparams so is too general. Instead of changing the name variable, add the docs for the function describing that there are the url params to filter

Suggested change
function fetchLicensesData(id, currentPage, urlParams) {
function fetchLicensesData(id, currentPage, filtersData) {

Copy link
Contributor

Choose a reason for hiding this comment

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

@01001110J you can name it same as the dispatch part, no problem with that

src/features/Licenses/LicensesFilters/index.jsx Outdated
dispatch(updateFilters(formJson));
dispatch(updateCurrentPage(initialPage));
licenseData.current = licenseOptions;
dispatch(fetchLicensesData(institution.id, initialPage, urlParams));
Copy link
Contributor

Choose a reason for hiding this comment

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

url params name is too general and I think this url params are only for filters

Suggested change
dispatch(fetchLicensesData(institution.id, initialPage, urlParams));
dispatch(fetchLicensesData(institution.id, initialPage, urlParamsFilters));

@01001110J 01001110J requested a review from AuraAlba February 22, 2024 21:50
@01001110J
feat: Filter by curse name and license name
9320ee2 
fix: Bug with missing options

fix: Unit test and button validation

fix: All broken test

fix:  src/features/Licenses/LicensesFilters/index.jsx

Co-authored-by: Aura Milena Alba <[email protected]>

fix: src/features/Licenses/LicensesFilters/__test__/index.test.jsx

Co-authored-by: Andrés Felipe Bermúdez Mendoza <[email protected]>

fix: src/features/Licenses/data/_test_/redux.test.jsx

Co-authored-by: Andrés Felipe Bermúdez Mendoza <[email protected]>

fix: Bug on change institution

refactor: Request params name
@01001110J 01001110J merged commit a081a51 into master Feb 23, 2024
2 checks passed
@01001110J 01001110J deleted the vue/PADV-1007 branch February 23, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AuraAlba AuraAlba AuraAlba approved these changes

@anfbermudezme anfbermudezme anfbermudezme approved these changes

Assignees

@01001110J 01001110J

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@01001110J @anfbermudezme @AuraAlba