Skip to content
This repository was archived by the owner on Sep 19, 2021. It is now read-only.

Conversation

@macrael
Copy link
Contributor

@macrael macrael commented Sep 6, 2019

Description

To simplify our db situation, this PR removes the rest of the old go-pg based database code. The last stuff that it was doing related to accounts has been implemented in simplestore using the new patterns. Getting rid of the last vestiges of the db code feels good!

There are a couple things left to do:

  • add Transmission support to simplestore
  • re-engage the Account.validate code on account save.

Checklist for Reviewer

  • Review code changes
  • Review changes for Database effects
  • Verify change works in IE browser

More details about this can be found in docs/review.md

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #1844 into develop will decrease coverage by 4.28%.
The diff coverage is 88.76%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1844      +/-   ##
===========================================
- Coverage    88.25%   83.97%   -4.28%     
===========================================
  Files          887      911      +24     
  Lines        20223    21501    +1278     
  Branches      3030     3219     +189     
===========================================
+ Hits         17846    18054     +208     
- Misses        1777     2478     +701     
- Partials       600      969     +369
Flag Coverage Δ
#backend 72.28% <88.76%> (-10.87%) ⬇️
#frontend 88.98% <ø> (-1.67%) ⬇️
Impacted Files Coverage Δ
api/http/attachment.go 89.38% <ø> (ø) ⬆️
api/http/status.go 69.23% <ø> (ø) ⬆️
api/http/save.go 83.33% <ø> (ø) ⬆️
api/http/form.go 68.97% <ø> (ø) ⬆️
api/http/refresh.go 100% <ø> (ø) ⬆️
api/simplestore/sessions.go 82.14% <ø> (-1.19%) ⬇️
api/simplestore/attachments.go 66.67% <0%> (ø) ⬆️
api/admin/reject.go 66.67% <100%> (-0.78%) ⬇️
api/admin/submit.go 70% <100%> (+2%) ⬆️
api/http/basic_auth.go 81.4% <100%> (-0.42%) ⬇️
... and 260 more

Continue to review full report at Codecov.

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

@macrael macrael marked this pull request as ready for review September 11, 2019 05:42
@macrael
Copy link
Contributor Author

macrael commented Sep 11, 2019

@ryanhofdotgov I've got to go to bed and be done. This PR is big and good, however, the merge into the mainline is still big and to be done. I've gotten started on it but I'm not done. I would really like to get this in before we roll off, but understand if we want to code freeze instead.

I think that if you're planning to cut a full release and do a merge then it's probably worth waiting on this code, I should probably be the person to do the merge since I've been thinking about it. But I'd love for you to review it and I can address comments when I'm back.

Take Care,

Copy link
Contributor

@ryanhofdotgov ryanhofdotgov left a comment

Choose a reason for hiding this comment

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

This is really nice to clean up – thanks! Aside from putting that code in a transaction, my main request on this one is that we have a full round of manual testing using the downstream fork this week; i.e., merge it in a downstream branch, test it out ahead of our end of sprint. And that @kdolan-soliel understands the motivation and approach here.

func (s SimpleStore) SetAccountPasswordHash(account api.Account) error {

// delete an existing one, if it exists
deleteQuery := `DELETE FROM basic_auth_memberships WHERE account_id = $1`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not super important since this is code that is just used for dev, but I'd put these 2 statements in a transaction.

@ryanhofdotgov ryanhofdotgov self-requested a review October 1, 2019 08:47
@ryanhofdotgov ryanhofdotgov dismissed their stale review October 1, 2019 08:48

Doesn't affect production, can be done in follow-up

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants