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.

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

Consolidate logic for searching sources and provide pagination framework #6764

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nabla-c0d3
Copy link
Contributor

@nabla-c0d3 nabla-c0d3 commented Mar 5, 2023

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).
  • 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 Pagination #2067 and Support API pagination #5073
  • Removes g.source as it was barely used anymore, to help with Reduce or eliminate the use of flask.g thread locals #6203.
  • Implements a SourceFactory to consolidate how to create a source in the unit tests.

@cfm cfm self-assigned this Mar 7, 2023
@cfm
Copy link
Member

cfm commented Mar 13, 2023

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.

@nabla-c0d3
Copy link
Contributor Author

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

@nabla-c0d3 nabla-c0d3 force-pushed the #6667-consolidate-sources-search branch from 752f589 to c714e25 Compare March 18, 2023 10:37
@nabla-c0d3
Copy link
Contributor Author

@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!

@cfm
Copy link
Member

cfm commented Mar 27, 2023

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.

@nabla-c0d3 nabla-c0d3 force-pushed the #6667-consolidate-sources-search branch from 1c5ad91 to 7de2bc8 Compare March 27, 2023 20:58
@nabla-c0d3 nabla-c0d3 closed this Apr 1, 2023
@nabla-c0d3 nabla-c0d3 reopened this Apr 1, 2023
@nabla-c0d3
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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?

@cfm
Copy link
Member

cfm commented Apr 11, 2023

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!

@nabla-c0d3 nabla-c0d3 force-pushed the #6667-consolidate-sources-search branch from 7de2bc8 to 12b6e6d Compare April 13, 2023 20:52
@nabla-c0d3
Copy link
Contributor Author

@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?

@cfm
Copy link
Member

cfm commented Apr 19, 2023 via email

@nabla-c0d3
Copy link
Contributor Author

nabla-c0d3 commented Apr 21, 2023

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

@nabla-c0d3
Copy link
Contributor Author

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

@nabla-c0d3
Copy link
Contributor Author

@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 nabla-c0d3 force-pushed the #6667-consolidate-sources-search branch from 93372fe to 57b7389 Compare April 22, 2023 15:44
@nabla-c0d3 nabla-c0d3 marked this pull request as ready for review April 22, 2023 16:19
@nabla-c0d3 nabla-c0d3 requested a review from a team as a code owner April 22, 2023 16:19
@cfm
Copy link
Member

cfm commented May 9, 2023 via email

@cfm
Copy link
Member

cfm commented Oct 3, 2023

@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 cfm added this to the SecureDrop 2.8.0 milestone Oct 3, 2023
@nabla-c0d3
Copy link
Contributor Author

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 !

@zenmonkeykstop zenmonkeykstop removed this from the SecureDrop 2.9.0 milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Journalist Interface and API return different subsets of Sources
3 participants