NIAEFEUP / nijobs-fe

[FRONTEND] A platform for companies to advertise their job opportunities to students
GNU General Public License v3.0
23 stars 3 forks source link

Show hidden offers to admin #291

Open diogofonte opened 1 year ago

diogofonte commented 1 year ago

Closes #236

Naapperas commented 1 year ago

@CarlosMealha @diogofonte any updates on this?

diogofonte commented 1 year ago

@CarlosMealha @diogofonte any updates on this?

We were waiting for opinions about the divs that are causing extra space in mobile view search, but the button "show hidden offers" is ready. This is not the not the principal objective of this PR, but the problem affects directly our issue.

Captura_de_ecra_2022-11-20_as_09 40 10 Captura_de_ecra_2022-11-20_as_09 40 32

Captura_de_ecra_2022-11-20_as_09 42 02

CarlosMealha commented 1 year ago

@CarlosMealha @diogofonte any updates on this?

Also, is there any endpoint responsible to fetch hidden offers or we need to filter the offers in order to specify them ?

Naapperas commented 1 year ago

@CarlosMealha @diogofonte any updates on this?

Also, is there any endpoint responsible to fetch hidden offers or we need to filter the offers in order to specify them ?

the GET /offers endpoint already handles this: you need to pass a query value named showHidden, if I recall correctly, and set it to true in order to see hidden offers (if you have permission to do so of course).

If you are asking in the context of this PR, the feature that needs to be implemented is the possibility for admins to pass that showHidden value, which is currently not possible. Maybe I misunderstood your question.

Naapperas commented 1 year ago

@CarlosMealha @diogofonte any updates on this?

We were waiting for opinions about the divs that are causing extra space in mobile view search, but the button "show hidden offers" is ready. This is not the not the principal objective of this PR, but the problem affects directly our issue.

Captura_de_ecra_2022-11-20_as_09 40 10 Captura_de_ecra_2022-11-20_as_09 40 32

Captura_de_ecra_2022-11-20_as_09 42 02

I think I looked around a while ago and recall this being an issue in the MultiOptionAutocomplete component, not that one, but i might be wrong. If the problem is in the component you showed maybe you could replace the React Fragment (the <></> thing) with null and see if it works.

CarlosMealha commented 1 year ago

@CarlosMealha @diogofonte any updates on this?

Also, is there any endpoint responsible to fetch hidden offers or we need to filter the offers in order to specify them ?

the GET /offers endpoint already handles this: you need to pass a query value named showHidden, if I recall correctly, and set it to true in order to see hidden offers (if you have permission to do so of course).

If you are asking in the context of this PR, the feature that needs to be implemented is the possibility for admins to pass that showHidden value, which is currently not possible. Maybe I misunderstood your question.

Yes, my question was about the endpoint specifically, and your answer cleared my doubts. Thank you!

CarlosMealha commented 1 year ago

Okay, me and Diogo made the show hidden toggle on admin account working properly

Naapperas commented 1 year ago

Since there is no backend to test this (at the moment), please include a small video demonstrating this feature.

Naapperas commented 1 year ago

Also, beware of the upstream changes, you'll need to rebase this branch with develop later.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 70.37% and project coverage change: -0.21 :warning:

Comparison is base (e2c6463) 89.05% compared to head (5626b67) 88.84%.

:exclamation: Current head 5626b67 differs from pull request most recent head 1d046e2. Consider uploading reports for the commit 1d046e2 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #291 +/- ## =========================================== - Coverage 89.05% 88.84% -0.21% =========================================== Files 176 175 -1 Lines 3390 3345 -45 Branches 851 845 -6 =========================================== - Hits 3019 2972 -47 - Misses 371 373 +2 ``` | [Impacted Files](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP) | Coverage Δ | | |---|---|---| | [...ltsArea/SearchResultsWidget/SearchResultsMobile.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2NvbXBvbmVudHMvSG9tZVBhZ2UvU2VhcmNoUmVzdWx0c0FyZWEvU2VhcmNoUmVzdWx0c1dpZGdldC9TZWFyY2hSZXN1bHRzTW9iaWxlLmpz) | `80.00% <ø> (ø)` | | | [...mponents/HomePage/SearchArea/useUrlSearchParams.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2NvbXBvbmVudHMvSG9tZVBhZ2UvU2VhcmNoQXJlYS91c2VVcmxTZWFyY2hQYXJhbXMuanM=) | `89.79% <16.66%> (-8.45%)` | :arrow_down: | | [...SearchArea/AdvancedSearch/AdvancedSearchDesktop.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2NvbXBvbmVudHMvSG9tZVBhZ2UvU2VhcmNoQXJlYS9BZHZhbmNlZFNlYXJjaC9BZHZhbmNlZFNlYXJjaERlc2t0b3AuanM=) | `91.66% <75.00%> (-8.34%)` | :arrow_down: | | [.../SearchArea/AdvancedSearch/AdvancedSearchMobile.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2NvbXBvbmVudHMvSG9tZVBhZ2UvU2VhcmNoQXJlYS9BZHZhbmNlZFNlYXJjaC9BZHZhbmNlZFNlYXJjaE1vYmlsZS5qcw==) | `77.41% <75.00%> (-0.36%)` | :arrow_down: | | [...age/SearchArea/AdvancedSearch/useAdvancedSearch.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2NvbXBvbmVudHMvSG9tZVBhZ2UvU2VhcmNoQXJlYS9BZHZhbmNlZFNlYXJjaC91c2VBZHZhbmNlZFNlYXJjaC5qcw==) | `96.77% <83.33%> (-3.23%)` | :arrow_down: | | [src/actions/searchOffersActions.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2FjdGlvbnMvc2VhcmNoT2ZmZXJzQWN0aW9ucy5qcw==) | `100.00% <100.00%> (ø)` | | | [src/components/HomePage/SearchArea/SearchArea.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL2NvbXBvbmVudHMvSG9tZVBhZ2UvU2VhcmNoQXJlYS9TZWFyY2hBcmVhLmpz) | `100.00% <100.00%> (+2.12%)` | :arrow_up: | | [src/reducers/searchOffersReducer.js](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP#diff-c3JjL3JlZHVjZXJzL3NlYXJjaE9mZmVyc1JlZHVjZXIuanM=) | `100.00% <100.00%> (+3.63%)` | :arrow_up: | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/NIAEFEUP/nijobs-fe/pull/291/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NIAEFEUP)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

CarlosMealha commented 1 year ago

Codecov Report

Base: 88.98% // Head: 88.96% // Decreases project coverage by -0.03% warning

Coverage data is based on head (6e15449) compared to base (5f96cb4). Patch coverage: 85.71% of modified lines in pull request are covered.

mega This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files

umbrella View full report at Codecov. loudspeaker Do you have feedback about the report comment? Let us know in this issue.

We will take care of this as the feature for the related showHidden URL filter are still in progress and the corresponding tests

diogofonte commented 1 year ago

Now the showHidden attribute is present in the URL

Video demonstrating the final result:

https://user-images.githubusercontent.com/93329645/221963200-2f3a64a6-0b5a-45e8-87af-69a6ca504763.mp4

BrunoRosendo commented 1 year ago

I'll leave the code review for the others but I think the result looks neat!

diogofonte commented 1 year ago

We noticed that it was not possible to reset the show hidden offers field with the button "reset advanced fields". Now it's possible.

Video:

https://user-images.githubusercontent.com/93329645/223278017-b34c24a2-dece-4fc1-b646-373052737ab2.mp4

Naapperas commented 1 year ago

Now the showHidden attribute is present in the URL

Video demonstrating the final result:

~This looks awesome, great job. I just had an idea: perhaps we should completely remove the showHidden field from the URL if it is not present. Since this is an admin only thing, its a little bit of "secrecy" (in lack of a better term) we had to this feature. Not mandatory tho, this is great as it is. What do you think @CarlosMealha @diogofonte @BrunoRosendo~

I just now saw that this was tackled in the video following, ignore this comment.

diogofonte commented 1 year ago

Now the showHidden attribute is present in the URL Video demonstrating the final result:

~This looks awesome, great job. I just had an idea: perhaps we should completely remove the showHidden field from the URL if it is not present. Since this is an admin only thing, its a little bit of "secrecy" (in lack of a better term) we had to this feature. Not mandatory tho, this is great as it is. What do you think @CarlosMealha @diogofonte @BrunoRosendo~

I just now saw that this was tackled in the video following, ignore this comment.

Yes, we changed it to follow the flow already implemented. We need to see what's going wrong with the tests and we think that after that it is ready to close and merge.

Naapperas commented 1 year ago

Also, don't forget to rebase.