fix: bug for searching by proposal ID failing to find proposal due to whitespace#1345
fix: bug for searching by proposal ID failing to find proposal due to whitespace#1345YufanKambang wants to merge 17 commits intodevelopfrom
Conversation
|
Found a query function that is related to bug, I think making sure that the input string if considered to be proposal IDs should be trimmed/normalised/validated before being passed as a query parameter in search proposals function |
|
Found the function However the |
|
Have made the fix, but yet to test it. Having issues with logging into office.localhost and user.localhost. |
|
Fixing my local set up of the ICD system to be able to test the code changes, and assess that it actually fixes the whitespace bug |
mutambaraf
left a comment
There was a problem hiding this comment.
Just a some few comments and can we also have tests e2e to this bug or backend unit tests.
There was a problem hiding this comment.
Would be good if this change was done for the instrument scientist table too
There was a problem hiding this comment.
I do not have the instrument scientist role. Is it the same as the experiment scientists?
There was a problem hiding this comment.
I was looking into this and the way the proposal id get passed to be turned into a query is a bit different for instrument-scientist compared to user-officers. I think it would help to put this into a different issue, that I can pick up
There was a problem hiding this comment.
I have made the update so that the instrument/experimental scientist table also does not get effected by whitespace. tested it on localhost using the role experimental scientist
There was a problem hiding this comment.
For the roles user-officer, the searchText that they put into the search bar int he proposal tables is directly used for the view query.
However, for the role instrument-scientist the searchText is put into a filter class called ProposalsFilter, and this ProposalsFilter is passed into the method getInstrumentScientistProposals and so we have to update the ProposalsFilter.text field with the trimmed searchtext that the intrument/experimental scientist will input into the intrument/experimental scientist proposal tables.
…s://github.com/UserOfficeProject/user-office-core into 1211-bug-when-whitespace-in-proposal-id-search
I have added a unit test for the role user-officer, for the whitespace bug. |
|
The unit test produced actually tests the wrong function that is being called, as we are not trying to get a proposal from the database, but to get filter the view from the user-officer table using the |
|
I have tried to test this locally but I have failed facing an import error with |
|
I also noticed no instructions on how to run unit and e2e tests locally on your work machine in |
| first?: number, | ||
| offset?: number | ||
| ) { | ||
| const cleanText = filter?.text?.trim(); |
There was a problem hiding this comment.
I would recommend trim to happen in datasource layer during sql query rather than the business logic layer here.
Let me know if you have any concerns
There was a problem hiding this comment.
This one is I have not been able to successfully move it to the datasource layer
I placed console.logs but have not been able to see them in any container logs.
How should I be handling this object filter for the @Authorized([Roles.INSTRUMENT_SCIENTIST, Roles.INTERNAL_REVIEWER]) proposal view as they have a whole object that gets passed compared to the string for @Authorized([Roles.USER_OFFICER])?
There was a problem hiding this comment.
This has now been fixed, the blocker was that the datasource layer that the fix was being applied to was the wrong one, it should have been StfcProposalDataSource.ts as that is the actual datasource layer being used for this proposal view.
|
I have been unable to push these changes due to the pre-commit linter. That is the reason why I have snipped my code changes |
Description
This pr is in response to the issue : UserOfficeProject/issue-tracker#1211
Motivation and Context
This is to improve user experience as well as to not cause confusion if they copied a proposal ID with a white space and see no proposals being shown
How Has This Been Tested
I have tested it using the localhost proposal portal, I first checked to see if the given proposal ID had a proposal in the system, when confirmed I tested it by adding whitespace to the start of the proposal ID, and the at the end of the ID.
Fixes
Changes
Depends on
Tests included/Docs Updated?