cds-snc / notification-planning-core

Project planning for GC Notify Core Team
0 stars 0 forks source link

Upgrade marshmallow dependency on api #70

Open sastels opened 1 year ago

sastels commented 1 year ago

Description

As a Notify developer I need to keep our dependancies up to date.

WHY are we building?

WHAT are we building?

We can upgrade our other dependancies

Acceptance Criteria

QA Steps

jimleroyer commented 1 year ago

I started on the task and replicate GDS did in their upgrade, minus a few differences.

sastels commented 1 year ago

work progresses, no big issues found so far

jimleroyer commented 1 year ago

I continued work yesterday on this. I am in the weeds with it as I applied most changes but now I need to debug. There is some code that does not align well with the strict default required by the schema definition by marshmallow, which GDS did but GCNotify didn't over the past years since the fork.

jimleroyer commented 1 year ago

Made some progress but moving back to the shelves as we have a higher priority task to tackle.

jimleroyer commented 1 year ago

Made further changes to the schema.

jimleroyer commented 1 year ago

I am at 99% test working, have to review the remaining 1% and then test locally extensively.

ben851 commented 1 year ago
ben851 commented 1 year ago
sastels commented 1 year ago

branch / PR https://github.com/cds-snc/notification-api/pull/1844

jimleroyer commented 1 year ago

Jimmy and Steve worked on the task and got all automated tests to run. Next step is to run locally and test it out.

jimleroyer commented 1 year ago

Jimmy working on his local setup; his login is not working.

sastels commented 1 year ago

Steve will try using this branch to create a new user (where Jimmy is having an issue with the verification email not being sent)

jimleroyer commented 1 year ago

Steve and Jimmy made it work after finding a few related and unrelated issues. Jimmy to test more thoroughly.

ben851 commented 1 year ago

Jimmy performed some tests and broke Admin. Jimmy will re-evaluate his fix to ensure existing functionality doesn't break.

There are some fields relating to mail/letter functionality that are still being referenced in admin that we tried to remove, but caused issues.

New card to remove letters could be created to tackle this reference removal.

jimleroyer commented 1 year ago

Tested the application locally, fixed a bug but now it seems to be working good. The PR would need a review.

jimleroyer commented 1 year ago

Andrew is taking a look at the PR for review. We might do a bug bash after deploying it to staging.

jimleroyer commented 12 months ago

Jimmy to reply to Andrew's questions PR.

jimleroyer commented 12 months ago

Answered to Andrew's question and waiting for this reply. Will tested and approved the PR so it would be ready to go.

sastels commented 11 months ago

Still being reviewed

sastels commented 11 months ago

good idea to do a bug bash on staging before merging to prod

jimleroyer commented 11 months ago

Bug bash is scheduled for tomorrow at 13:30 EST.