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

fix: Force All Contents Table Searches to Lower Case #2159

Closed
wants to merge 4 commits into from
Closed

fix: Force All Contents Table Searches to Lower Case #2159

wants to merge 4 commits into from
+16 −0

Conversation

nicholas-codecov
Copy link
Contributor

Description

This PR closes #2157 by forcing all search param strings to lower case before sending to the API so that if the user so the search value param matches that of the requesting user.

Notable Changes

  • Update all *ContentsTables hooks to send search param string to lower case.

@nicholas-codecov
update useRepoCommitContentsTable to send search param to lower case …

Verified

This commit was signed with the committer’s verified signature.
RulaKhaled Rola Abuhasna
GPG key ID: A5E9CDAB9188A2FE
Verified
Learn about vigilant mode
Loading
Loading status checks…
2aee6eb 
…if present
@nicholas-codecov
update useRepoPullContentsTable to send search param to lower case if…

Verified

This commit was signed with the committer’s verified signature.
RulaKhaled Rola Abuhasna
GPG key ID: A5E9CDAB9188A2FE
Verified
Learn about vigilant mode
Loading
Loading status checks…
71ff11f 
… present
@nicholas-codecov
update useRepoBranchContentsTable to send search param to lower case …

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
d47ddf4 
…if present
@nicholas-codecov
Merge branch 'main' into gazebo-2157-file-search-is-case-sensitive-it…

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Loading
Loading status checks…
1588424 
…-should-not-be
@codecov-staging
Copy link

codecov-staging bot commented Jul 26, 2023 •
edited
Loading

Codecov Report

Merging #2159 (1588424) into main (e2094e5) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2159      +/-   ##
==========================================
- Coverage   97.68%   97.66%   -0.03%     
==========================================
  Files         690      690              
  Lines        7697     7703       +6     
  Branches     1819     1822       +3     
==========================================
+ Hits         7519     7523       +4     
- Misses        176      177       +1     
- Partials        2        3       +1
Files Changed Coverage Δ
...ilFileExplorer/hooks/useRepoCommitContentsTable.js 100.00% <100.00%> (ø)
...ute/FileExplorer/hooks/useRepoPullContentsTable.js 100.00% <100.00%> (ø)
...e/FileExplorer/hooks/useRepoBranchContentsTable.js 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2094e5...1588424. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 26, 2023 •
edited
Loading

Codecov Report

Merging #2159 (1588424) into main (e2094e5) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2159     +/-   ##
=======================================
- Coverage   97.69   97.66   -0.03     
=======================================
  Files        690     690             
  Lines       7697    7703      +6     
  Branches    1819    1822      +3     
=======================================
+ Hits        7519    7523      +4     
- Misses       176     177      +1     
- Partials       2       3      +1
Files Changed Coverage Δ
...ilFileExplorer/hooks/useRepoCommitContentsTable.js 100.00% <100.00%> (ø)
...ute/FileExplorer/hooks/useRepoPullContentsTable.js 100.00% <100.00%> (ø)
...e/FileExplorer/hooks/useRepoBranchContentsTable.js 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2094e5...1588424. Read the comment docs.

@netlify
Copy link

netlify bot commented Jul 26, 2023

✅ Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit 1588424
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/64c152947fb1a60008555de2
😎 Deploy Preview https://deploy-preview-2159--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

adrian-codecov
adrian-codecov approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@adrian-codecov adrian-codecov left a comment

Choose a reason for hiding this comment

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

Did you play around with this to ensure we're not case sensitive on the API? I don't think we are but worth double checking

@nicholas-codecov
Copy link
Contributor Author

Did you play around with this to ensure we're not case sensitive on the API? I don't think we are but worth double checking

We definitely are limited on the API side as well it seems.

@nicholas-codecov nicholas-codecov mentioned this pull request Aug 3, 2023
Closed
@nikosatwork
Copy link

Hi @nicholas-codecov. I have been following this PR - is there any progress?

It is not clear if search is still case sensitive, however it maps to lowercase search? For example, if there are these two files:
Class.cs
Anotherclass.cs
and in the search it is typed:
Class
then would this result in both files be listed or just Anotherclass.cs?

@nicholas-codecov
Copy link
Contributor Author

Hi @nikosatwork!

I'm not 100% certain as to the logic on the backend and how they're doing the filtering in the DB query. This PR here makes things worse as the backend is searching in a case-sensitive manner currently. The progress can be followed here on those changes. Once they're merged in this PR may not actually be needed because they will handle all of the conversion to lower case and filtering.

@nikosatwork
Copy link

I think that PR makes more sense - thank you for the update Nicholas

@nicholas-codecov
Copy link
Contributor Author

No problem!

@nicholas-codecov
Copy link
Contributor Author

Hey @nikosatwork, just giving you a quick heads up that this issue has been resolved in the API and should be functioning fully.

@nicholas-codecov nicholas-codecov deleted the gazebo-2157-file-search-is-case-sensitive-it-should-not-be branch August 14, 2023 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian-codecov adrian-codecov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

File search is case sensitive, it should not be
3 participants
@nicholas-codecov @nikosatwork @adrian-codecov