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

Sync logic #27

Merged
merged 8 commits into from
Sep 21, 2018
Merged

Sync logic #27

merged 8 commits into from
Sep 21, 2018

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Sep 13, 2018

This PR contains a solution for #5. However, it's a work in progress and given that I'm "new" to this project I'm keen to get feedback about style/approach.

This is stacked on PR #23.

Questions:

  • I've included unit tests. What about integration tests. Thoughts / approaches please?
  • I'm using the dependency injection pattern -- I'm assuming the app will make the api and database session objects these functions depend upon.
  • The concept of a "reply" is not addressed, but I imagine will need to be in a similar fashion to sources and submissions. It would be trivial to add, so happy to do so as additional work on this PR.

@ntoll
Copy link
Contributor Author

ntoll commented Sep 13, 2018

Dammit... the build has failed because sdclientapi isn't available. This is the first time I've used pipenv so I'm not too sure how to include something from GitHub rather than PyPI. If you point me at instructions I'll fix up tomorrow. Thanks.

@kushaldas
Copy link
Contributor

kushaldas commented Sep 13, 2018

@ntoll try this::

pipenv install git+https://github.com/freedomofpress/securedrop-sdk.git#egg=securedrop-sdk

@redshiftzero redshiftzero self-requested a review September 13, 2018 20:30
@ntoll
Copy link
Contributor Author

ntoll commented Sep 14, 2018

Thanks @kushaldas -- CI is working now. :-)

@redshiftzero
Copy link
Contributor

hey @ntoll gave this a look over today, super clear implementation, this is exactly what we're lookin for! let's add the replies to this, and one note on that to bear in mind (apologies if you thought about/realized this already): as journalist users are observed in replies we want to add the journalist username/UUID to the database (and usernames can change so we need to update them too).

@ntoll
Copy link
Contributor Author

ntoll commented Sep 17, 2018

@redshiftzero OK... added reply and user handling.

not adding a separate migration because we're going to squash
all of these prior to initial release
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

looks great!

@@ -21,7 +21,7 @@
"""
import logging
from dateutil.parser import parse
from securedrop_client.models import Source, Submission
from securedrop_client.models import Source, Submission, Reply, User
Copy link
Contributor

Choose a reason for hiding this comment

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

I foresee confusion between the sqlalchemy models and the SDK objects (not introduced in this PR, just a thought). Maybe we can change the names of the sqlalchemy objects to SourceDatabaseObject or something similar for the sake of being explicit (I don't love that name but you get the idea)

@redshiftzero redshiftzero merged commit 64adc5e into freedomofpress:master Sep 21, 2018
@eloquence eloquence mentioned this pull request Sep 21, 2018
legoktm pushed a commit that referenced this pull request Dec 11, 2023
Adds new sha256sums for PyYAML 5.1 and new release
legoktm pushed a commit that referenced this pull request Dec 15, 2023
minor fixes to stabilize the dev env
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants