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/refactor submission approved #2490

Merged
merged 9 commits into from
Jan 8, 2024
Merged

Fix/refactor submission approved #2490

merged 9 commits into from
Jan 8, 2024

Conversation

clari182
Copy link
Collaborator

@clari182 clari182 commented Dec 13, 2023 •
edited
Loading

@netlify Netlify
Copy link

netlify bot commented Dec 13, 2023 •
edited
Loading

✅ Deploy Preview for staging-aiid canceled.

Name Link
🔨 Latest commit a4cec26
🔍 Latest deploy log https://app.netlify.com/sites/staging-aiid/deploys/6596b687071e410008489375

@clari182 clari182 marked this pull request as draft December 13, 2023 15:56
@clari182 clari182 requested a review from pdcp1 December 14, 2023 18:01
@clari182 clari182 marked this pull request as ready for review December 14, 2023 18:01
pdcp1
pdcp1 approved these changes Dec 15, 2023
View reviewed changes
Copy link
Collaborator

@pdcp1 pdcp1 left a comment

Choose a reason for hiding this comment

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

The code LGTM and the functionality work as expected in Clara's environment

@pdcp1 pdcp1 assigned pdcp1, cesarvarela and kepae and unassigned pdcp1 Dec 15, 2023
@pdcp1
Copy link
Collaborator

pdcp1 commented Dec 15, 2023

@cesarvarela @kepae This PR should be merged after this one #2459 because this last one has a test code refactor.

@clari182 clari182 mentioned this pull request Dec 20, 2023
Merged
@clari182 clari182 requested a review from pdcp1 January 4, 2024 19:30
@clari182
Copy link
Collaborator Author

clari182 commented Jan 4, 2024

I merged staging and there were some conflicts so another review is suggested

pdcp1
pdcp1 reviewed Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@pdcp1 pdcp1 left a comment

Choose a reason for hiding this comment

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

@clari182 did you explicitly change the /functions/processNotifications.js file or are the changes part of the Staging merge?
I have the same question for processNewPromotionsNotifications.cy.js file.

@clari182
Copy link
Collaborator Author

clari182 commented Jan 5, 2024

@pdcp1 I explicitly changed them. For /functions/processNotifications.js I added the check to only send emails to unique userIds, this was done for the other subscriptions but not for submittion-promoted. The other change in that file seems to be a formatting one which I can remove if you want.
For processNewPromotionsNotifications.cy.js I had to update the tests in order to adapt to the code change.

@clari182 clari182 requested a review from pdcp1 January 5, 2024 11:59
pdcp1
pdcp1 approved these changes Jan 5, 2024
View reviewed changes
Copy link
Collaborator

@pdcp1 pdcp1 left a comment

Choose a reason for hiding this comment

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

LGTM. After merging this into Staging we should do a test there.

@kepae
Copy link
Collaborator

kepae commented Jan 8, 2024

Stellar. Let's merge and test, and we'll re-enable notifications for this push.

@kepae kepae merged commit 5964151 into responsible-ai-collaborative:staging Jan 8, 2024
7 of 8 checks passed
@kepae kepae mentioned this pull request Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdcp1 pdcp1 pdcp1 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@clari182 @pdcp1 @kepae @cesarvarela