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

Show hidden offers to admin #291

Open
wants to merge 58 commits into
base: develop
Choose a base branch
from
Open

Conversation

diogofonte
Copy link
Member

@diogofonte diogofonte commented Dec 13, 2022

Closes #236

@Naapperas
Copy link
Member

@CarlosMealha @diogofonte any updates on this?

@diogofonte
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@Naapperas
Copy link
Member

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

@CarlosMealha CarlosMealha force-pushed the feature/showHiddenToAdmin branch 2 times, most recently from ed77758 to 598f1ae Compare January 24, 2023 16:36
Copy link
Member

@Naapperas Naapperas left a comment

Choose a reason for hiding this comment

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

There should be tests regarding the UI and Request making parts of this feature, not just the Redux store one.

@Naapperas
Copy link
Member

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

src/actions/searchOffersActions.spec.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023

Codecov Report

Patch coverage: 70.37% and project coverage change: -0.21 ⚠️

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

❗ 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
@@             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 Coverage Δ
...ltsArea/SearchResultsWidget/SearchResultsMobile.js 80.00% <ø> (ø)
...mponents/HomePage/SearchArea/useUrlSearchParams.js 89.79% <16.66%> (-8.45%) ⬇️
...SearchArea/AdvancedSearch/AdvancedSearchDesktop.js 91.66% <75.00%> (-8.34%) ⬇️
.../SearchArea/AdvancedSearch/AdvancedSearchMobile.js 77.41% <75.00%> (-0.36%) ⬇️
...age/SearchArea/AdvancedSearch/useAdvancedSearch.js 96.77% <83.33%> (-3.23%) ⬇️
src/actions/searchOffersActions.js 100.00% <100.00%> (ø)
src/components/HomePage/SearchArea/SearchArea.js 100.00% <100.00%> (+2.12%) ⬆️
src/reducers/searchOffersReducer.js 100.00% <100.00%> (+3.63%) ⬆️

... and 11 files with indirect coverage changes

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

@CarlosMealha
Copy link
Contributor

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 diogofonte force-pushed the feature/showHiddenToAdmin branch 2 times, most recently from b3433d2 to 140ea74 Compare February 28, 2023 18:57
@diogofonte
Copy link
Member Author

Now the showHidden attribute is present in the URL

Video demonstrating the final result:

show_hidden_offers.mp4

@BrunoRosendo
Copy link
Member

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

Copy link
Member

@tomaspalma tomaspalma left a comment

Choose a reason for hiding this comment

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

Everything looks good and is working.

There is just a small detail that in terms of functionality doesn't really matter probably, but basically, if a user without privileges types showHidden=false or true, it shows a dot on the settings icon near the search bar, but then when we click it, in the advanced filters menu, there is obviously nothing there.

However, I believe this doesn't do anything because as far as I saw, the backend checks if a user has privileges or not to show the hidden offer and his an edge case that would probably never happen, so it's not that important.

Copy link
Member

@Naapperas Naapperas left a comment

Choose a reason for hiding this comment

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

Left a small comment.

@DoStini please review this code once more, since Github is picky about change requests 🙃

gridRowStart: 4,
gridColumnStart: 2,
},
spaceBtwn: {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of this: this class should have another name, and not one that simply re-states what it does style-wise.

@Naapperas
Copy link
Member

Also, don't forget to rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add show hidden to admin search filters
7 participants