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

feat/AB#74292 People picker #725

Open
wants to merge 35 commits into
base: 2.x.x
Choose a base branch
from
Open

feat/AB#74292 People picker #725

wants to merge 35 commits into from

Conversation

TaiKamilla
Copy link
Contributor

@TaiKamilla TaiKamilla commented Sep 4, 2023 •
edited by Matthis-M-ReliefApps
Loading

Description

In this PR the backend responds to the query GetPeople and send back some mock data

Useful links

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (refactor or addition to existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

image

Checklist:

( * == Mandatory )

  • * I have set myself as assignee of the pull request
  • * My code follows the style guidelines of this project
  • * Linting does not generate new warnings
  • * I have performed a self-review of my own code
  • * I have put the ticket for review, adding the oort-frontend team to the list of reviewers
  • * I have commented my code, particularly in hard-to-understand areas
  • * I have put JSDoc comment in all required places
  • * My changes generate no new warnings
  • * I have included screenshots describing my changes if relevant
  • * I have selected labels in the Pull Request, according to the changes with code brings
  • I have made corresponding changes to the documentation ( if required )
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@TaiKamilla TaiKamilla added the enhancement Existing feature label Sep 4, 2023
@TaiKamilla TaiKamilla requested a review from a team September 4, 2023 14:18
@TaiKamilla TaiKamilla self-assigned this Sep 4, 2023
@TaiKamilla TaiKamilla changed the title mock data feat/AB#74292_ABC-Create-people-picker-question Sep 4, 2023
@NathanHGit NathanHGit mentioned this pull request Sep 6, 2023
Open
16 tasks
@NathanHGit NathanHGit requested review from NathanHGit and removed request for a team September 6, 2023 08:44
NathanHGit
NathanHGit approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@NathanHGit NathanHGit left a comment

Choose a reason for hiding this comment

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

Looks to work fine

@MwanPygmay MwanPygmay changed the base branch from 2.1.x to beta September 26, 2023 08:47
@MwanPygmay MwanPygmay self-requested a review September 26, 2023 08:52
MwanPygmay
MwanPygmay approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@MwanPygmay MwanPygmay left a comment

Choose a reason for hiding this comment

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

  • Added support for people type
  • Added support for people metadata

@TaiKamilla TaiKamilla force-pushed the feat/AB#74292_ABC-Create-people-picker-question branch from f74545f to 6602426 Compare October 9, 2023 07:41
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 20, 2024
View reviewed changes
src/routes/upload/index.ts Outdated
true,
true
);
return res.status(200).send(insertRecordsMessage);

Check failure

Code scanning / CodeQL

Reflected cross-site scripting High

Cross-site scripting vulnerability due to a
user-provided value
Loading
.
src/utils/files/resourceExporter.ts Outdated
method: 'POST',
headers: this.axiosHeaders(),
data: {
query: metaQuery,

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query string depends on a
user-provided value
Loading
.
src/routes/email/index.ts Outdated
} catch (err) {
// Specific try / catch so we can know what the error is
logger.error(err.message, { stack: err.stack });
return res.status(500).send(err.message);

Check warning

Code scanning / CodeQL

Exception text reinterpreted as HTML Medium

Exception text
Loading
is reinterpreted as HTML without escaping meta-characters.
src/server/pullJobScheduler.ts Outdated
Comment on lines 416 to 419
const duplicateRecords = await RecordModel.find({
form: pullJob.convertTo,
$or: filters,
}).select(selectedFields);

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query object depends on a
user-provided value
Loading
.
@NathanHGit NathanHGit changed the base branch from beta to 2.x.x March 20, 2024 13:06
Matthis-M-ReliefApps
Matthis-M-ReliefApps requested changes Mar 29, 2024
View reviewed changes
Copy link
Collaborator

@Matthis-M-ReliefApps Matthis-M-ReliefApps left a comment

Choose a reason for hiding this comment

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

Not a real review but please implement the filter as explained in latest comment on https://dev.azure.com/WHOHQ/EMSSAFE/_workitems/edit/74292

@NathanHGit
Copy link
Contributor

@Matthis-M-ReliefApps

It has been implemented in the frontend to maintain the route generic
ReliefApplications/ems-frontend#1792

@AntoineRelief
Copy link
Collaborator

@Matthis-M-ReliefApps @MwanPygmay

I got some Axios errors when trying the feature, did you get some when testing on your side also?

@MwanPygmay
Copy link
Contributor

@AntoineRelief do you have the scope for common services?

@AntoineRelief
Copy link
Collaborator

AntoineRelief commented Apr 25, 2024 •
edited
Loading

@AntoineRelief do you have the scope for common services?

@MwanPygmay
Apart if it had changed, I think so yes

Doing the same request on Postman, I can get the data
I can't from the code thus

@Matthis-M-ReliefApps
Copy link
Collaborator

@AntoineRelief do you have the scope for common services?

@MwanPygmay Apart if it had changed, I think so yes

Doing the same request on Postman, I can get the data I can't from the code thus

I remember I also had some requests errors when I tried to start a review on it last week

Matthis-M-ReliefApps
Matthis-M-ReliefApps requested changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@Matthis-M-ReliefApps Matthis-M-ReliefApps left a comment

Choose a reason for hiding this comment

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

Seems all good except for the hardcoded string

src/utils/proxy/getPeople.ts Outdated Show resolved Hide resolved
Matthis-M-ReliefApps
Matthis-M-ReliefApps approved these changes May 21, 2024
View reviewed changes
Copy link
Collaborator

@Matthis-M-ReliefApps Matthis-M-ReliefApps left a comment

Choose a reason for hiding this comment

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

Perfection

@AntoineRelief AntoineRelief changed the title feat/AB#74292_ABC-Create-people-picker-question feat/AB#74292 People picker Jun 3, 2024
@AntoineRelief AntoineRelief marked this pull request as draft June 14, 2024 07:37
@MwanPygmay MwanPygmay marked this pull request as ready for review June 19, 2024 10:11
@AntoineRelief
Copy link
Collaborator

AntoineRelief commented Jun 26, 2024 •
edited
Loading

@Matthis-M-ReliefApps @MwanPygmay

looking at it again, I can see it's not yet complete:

  • history is not working fine
  • filtering in aggregations & layouts is not proposing any filter operator
  • export from resource is not working ( not the one from the grid, but from the resource page )

I can't merge it like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanHGit NathanHGit NathanHGit approved these changes

@AntoineRelief AntoineRelief AntoineRelief left review comments

@Matthis-M-ReliefApps Matthis-M-ReliefApps Matthis-M-ReliefApps approved these changes

@MwanPygmay MwanPygmay Awaiting requested review from MwanPygmay

Assignees

@TaiKamilla TaiKamilla

@NathanHGit NathanHGit

Labels
enhancement Existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TaiKamilla @NathanHGit @AntoineRelief @MwanPygmay @Matthis-M-ReliefApps