securedrop icon indicating copy to clipboard operation
securedrop copied to clipboard

Consolidate logic for searching sources and provide pagination framework

Open nabla-c0d3 opened this issue 2 years ago • 15 comments

Status

Ready for review.

Description of Changes

This PR does the following:

  • Provides an abstraction to consolidate the business logic around getting, searching and deleting sources, instead of having to write ORM code/queries directly in many locations within the code base (or having an unorganized/untested "utils" file).
    • I've called this layer "actions" but I'm open to other names.
    • The new"source actions" have full test coverage for searching and deleting sources.
    • Later, similar actions could be implemented for submissions, journalists, etc.
    • This fixes https://github.com/freedomofpress/securedrop/issues/6667
  • Implements a framework to set the stage for pagination in the journalist API and UI.
    • Sources can now be fetched per pages, via the SearchSourcesAction.
    • Future "search actions" (for submissions, etc.) will be able to use this pagination framework too.
    • This is a step forward for https://github.com/freedomofpress/securedrop/issues/2067 and https://github.com/freedomofpress/securedrop/issues/5073
  • Removes g.source as it was barely used anymore, to help with https://github.com/freedomofpress/securedrop/issues/6203.
  • Implements a SourceFactory to consolidate how to create a source in the unit tests.

nabla-c0d3 avatar Mar 05 '23 17:03 nabla-c0d3

It's exciting to see this draft, @nabla-c0d3. Thanks for working on it, and please let me know if there's anything you'd like to discuss along the way.

cfm avatar Mar 13 '23 19:03 cfm

@cfm Thank you! Nothing special to discuss so far. I will eventually get back to this and finish 👍 

nabla-c0d3 avatar Mar 14 '23 20:03 nabla-c0d3

@cfm Would you be able to "authorize" builds on CircleCI ? It says I'm "Unauthorized": https://app.circleci.com/pipelines/github/freedomofpress/securedrop?branch=pull%2F6764 Thanks!

nabla-c0d3 avatar Mar 27 '23 19:03 nabla-c0d3

That's puzzling, @nabla-c0d3, since recent commits from other contributors have run through CI just fine without special action on our part. We did have to rotate all our CircleCI‒GitHub credentials early in the new year. Could you do the same, via item (6) in https://support.circleci.com/hc/en-us/articles/360050273651-Builds-Unauthorized-due-to-contexts? I'll keep an eye out here.

cfm avatar Mar 27 '23 20:03 cfm

@cfm I've gone through step 6 but the problem is still there. I've sent an email to CircleCI Support; hopefully they can find what's wrong.

nabla-c0d3 avatar Apr 01 '23 13:04 nabla-c0d3

@cfm CircleCI support came back to me with the following explanation:

I was able to find the PR that caused the organization to use context. It would be great if you can check it out as well.https://github.com/freedomofpress/securedrop/pull/6739/files

securedrop_ci:
     jobs:
     - lint:
         context:
         - circleci-slack

Because of this change now the pipeline can only be run by a user part of member. It would be great if you can ask if them to use project's environment variables instead of context.

So it looks like this context is the cause of the problem?

nabla-c0d3 avatar Apr 05 '23 08:04 nabla-c0d3

Thanks for troubleshooting this, @nabla-c0d3. CI should pass on your branch, without failing Slack notifications, once you rebase from develop after #6776. Sorry for the hassle!

cfm avatar Apr 11 '23 19:04 cfm

@cfm I rebased my branch just now, but still seeing the same "Unauthorized" error on CIrcleCI: https://app.circleci.com/pipelines/github/freedomofpress/securedrop/5672

Based on the response from support tho, it seems like just using contexts will cause CI to not be run for members that are outside of the organization?

nabla-c0d3 avatar Apr 13 '23 20:04 nabla-c0d3

On Thu, Apr 13, 2023 at 01:58:31PM -0700, Alban Diquet wrote:

Based on the response from support tho, it seems like just using contexts will cause CI to not be run for members that are outside of the organization?

That would make sense, but after my fix in #6776 CI resumed on #6745 from outside @freedomofpress. I thought it might have to do with the timeline of how these pull requests were interleaved, but no: both this pull request and #6745 were opened before the context was introduced (merged) in #6739.

I was able to kick your CI off here in https://app.circleci.com/pipelines/github/freedomofpress/securedrop/5672/workflows/8432eaa0-2717-4c75-9676-e105c037f3db, and maybe it will work again on your next push....

cfm avatar Apr 19 '23 23:04 cfm

@cfm Unfortunately I just pushed and it is still happening (https://app.circleci.com/pipelines/github/freedomofpress/securedrop?branch=pull%2F6764)...

nabla-c0d3 avatar Apr 21 '23 20:04 nabla-c0d3

I wonder if this has to do with the fact that I have a CircleCI account attached to my GH account?

nabla-c0d3 avatar Apr 21 '23 21:04 nabla-c0d3

@cfm I had to enable CircleCI in my fork, but now it looks like everything's working 👍. I'm pretty sure the issue has to do with contributors who also have a CircleCI account.

nabla-c0d3 avatar Apr 22 '23 14:04 nabla-c0d3

On Sat, Apr 22, 2023 at 07:37:02AM -0700, Alban Diquet wrote:

@cfm I had to enable CircleCI in my fork, but now it looks like everything's working 👍. I'm pretty sure the issue has to do with contributors who also have a CircleCI account.

That makes sense. We've recently begun porting our CI configurations from CircleCI to GitHub Actions, so this edge-case shouldn't be around for much longer. Thanks for solving it in your case!

I should be able to get back to you with a review ETA next week.

cfm avatar May 09 '23 18:05 cfm

@nabla-c0d3, I apologize for our (very) slow response to this work. Based on our new roadmap for the rest of 2023 into 2024, we propose to slate this for SecureDrop 2.8. That would mean review later this year for release in the first quarter of 2024.

Our work to switch to Sequoia in v2.7 has already introduced some conflicts with your changes here. Those files should be stable enough as of the v2.7 release for you to resolve these conflicts once prior to review.

Let me know what you think! And thank you for your patience. If you'd like to chat with us at any point, we recently reopened our daily stand-up (https://github.com/freedomofpress/securedrop/wiki/Standup-Notes).

cfm avatar Oct 03 '23 01:10 cfm

we propose to slate this for SecureDrop 2.8. That would mean review later this year for release in the first quarter of 2024.

@cfm seems fine to me and worries !

nabla-c0d3 avatar Oct 15 '23 17:10 nabla-c0d3