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

Add support for extra_properties in per task instructions #659

Merged
merged 4 commits into from Aug 10, 2018

Conversation

Projects
None yet
6 participants
@pgiraud
Member

pgiraud commented Jul 9, 2017

Replaces #593

@pgiraud

This comment has been minimized.

Show comment
Hide comment
@pgiraud

pgiraud Jul 9, 2017

Member

@hunt3ri I updated my code so that it doesn't include any the migration scripts anymore.
I also fixed obvious typos which were indeed preventing the project creation.
However I didn't really get our point about the PUT API request. #593 (comment)
After some tests on my local instance, I can confirm that I'm able to create project in the different modes. I you don't please give me a procedure so that I can reproduce.

Member

pgiraud commented Jul 9, 2017

@hunt3ri I updated my code so that it doesn't include any the migration scripts anymore.
I also fixed obvious typos which were indeed preventing the project creation.
However I didn't really get our point about the PUT API request. #593 (comment)
After some tests on my local instance, I can confirm that I'm able to create project in the different modes. I you don't please give me a procedure so that I can reproduce.

@pgiraud pgiraud requested a review from hunt3ri Jul 9, 2017

@hunt3ri

There's a bug when user attempts to update their project.

Show outdated Hide outdated server/models/dtos/project_dto.py Outdated
@3vivekb

This comment has been minimized.

Show comment
Hide comment
@3vivekb

3vivekb Dec 13, 2017

This feature is pretty important for imports. It is something that exists in TM2. I hope somebody can test and get this deployed.

3vivekb commented Dec 13, 2017

This feature is pretty important for imports. It is something that exists in TM2. I hope somebody can test and get this deployed.

@bgirardot

This comment has been minimized.

Show comment
Hide comment
@bgirardot

bgirardot Dec 13, 2017

Collaborator

@3vivekb Hi vivek, I am not sure how to test this really. Any help would be appreciated on this PR. It will not really be able to be dealt with for at least a month or so as pierre is out of town and will not be able to look at it again before he returns.

Think you can help with it in some way? Test or provide a file and steps for testing? It is a feature I am totally not familiar with so I am not sure how to even test it.

Collaborator

bgirardot commented Dec 13, 2017

@3vivekb Hi vivek, I am not sure how to test this really. Any help would be appreciated on this PR. It will not really be able to be dealt with for at least a month or so as pierre is out of town and will not be able to look at it again before he returns.

Think you can help with it in some way? Test or provide a file and steps for testing? It is a feature I am totally not familiar with so I am not sure how to even test it.

@bgirardot

This comment has been minimized.

Show comment
Hide comment
@bgirardot

bgirardot Jul 3, 2018

Collaborator

Hi @pgiraud and @ethan-nelson Do you think we could revisit this PR and see about getting it merged in?

I am happy to test it well if we can resolve the conflicts, but I do not have the skills to resolve the conflicts.

This bumped up in priority due to the MS Building footprints. Several folks would like to do the building imports based on arbitrary geometries (census tracts) which is what this enables.

Collaborator

bgirardot commented Jul 3, 2018

Hi @pgiraud and @ethan-nelson Do you think we could revisit this PR and see about getting it merged in?

I am happy to test it well if we can resolve the conflicts, but I do not have the skills to resolve the conflicts.

This bumped up in priority due to the MS Building footprints. Several folks would like to do the building imports based on arbitrary geometries (census tracts) which is what this enables.

pgiraud added some commits Jun 20, 2017

Add support for extra_properties
in per task instructions
for arbitrary polygons
@pgiraud

This comment has been minimized.

Show comment
Hide comment
@pgiraud

pgiraud Jul 10, 2018

Member

Hi @bgirardot I just rebased the PR. Please let me know if it works.

Member

pgiraud commented Jul 10, 2018

Hi @bgirardot I just rebased the PR. Please let me know if it works.

@pgiraud

This comment has been minimized.

Show comment
Hide comment
@pgiraud

pgiraud Jul 30, 2018

Member

@bgirardot Did you get a chance to test this?

Member

pgiraud commented Jul 30, 2018

@bgirardot Did you get a chance to test this?

@bgirardot

This comment has been minimized.

Show comment
Hide comment
@bgirardot

bgirardot Jul 30, 2018

Collaborator

@pgiraud No, I apologize, I thought @ethan-nelson Was looking into this one Let me see about testing it later this week.

Collaborator

bgirardot commented Jul 30, 2018

@pgiraud No, I apologize, I thought @ethan-nelson Was looking into this one Let me see about testing it later this week.

@ethan-nelson

This comment has been minimized.

Show comment
Hide comment
@ethan-nelson

ethan-nelson Jul 30, 2018

Member

Yes I can test this week. Thanks!

Member

ethan-nelson commented Jul 30, 2018

Yes I can test this week. Thanks!

@@ -142,6 +143,7 @@ class Task(db.Model):
x = db.Column(db.Integer)
y = db.Column(db.Integer)
zoom = db.Column(db.Integer)
extra_properties = db.Column(db.Unicode)

This comment has been minimized.

@ethan-nelson

ethan-nelson Aug 5, 2018

Member

Note: this column already exists in the database (added through the migrations), but it wasn't mapped into the DB models.

@ethan-nelson

ethan-nelson Aug 5, 2018

Member

Note: this column already exists in the database (added through the migrations), but it wasn't mapped into the DB models.

@@ -56,6 +56,7 @@ class Project(db.Model):
license_id = db.Column(db.Integer, db.ForeignKey('licenses.id', name='fk_licenses'))
geometry = db.Column(Geometry('MULTIPOLYGON', srid=4326))
centroid = db.Column(Geometry('POINT', srid=4326))
task_creation_mode = db.Column(db.Integer, default=TaskCreationMode.GRID.value, nullable=False)

This comment has been minimized.

@ethan-nelson

ethan-nelson Aug 5, 2018

Member

Note: this column already exists in the database (added through the migrations), but it wasn't mapped into the DB models.

@ethan-nelson

ethan-nelson Aug 5, 2018

Member

Note: this column already exists in the database (added through the migrations), but it wasn't mapped into the DB models.

@ethan-nelson

Hi Pierre, thanks for working on this! I was able to successfully create new projects and call things like {x} and {y}.

One logistical issue may be blocking this (otherwise it is good to go). From my production database clone generated a while ago, it seems the task_creation_mode column was not populated...probably because that extra snippet in the migration script was not there previously. This won't be a problem for new databases, but it may be the case that we need to generate another alembic version running this to be sure those who did use the migration script have that flag set properly.

Let me tag up with the sysadmin on slack to verify this is the case on the live database and not just my clone.

ethan-nelson added some commits Aug 8, 2018

@ethan-nelson

This comment has been minimized.

Show comment
Hide comment
@ethan-nelson

ethan-nelson Aug 8, 2018

Member

Hi Pierre, I added a database revision to handle the case where a database has been migrated prior to your changes. In addition, I also added to the migration script the case where zoom values were not none so that projects would not have a blank value.

I ran the revision file and it worked for a local clone (the ratio was about 9 to 1 for non-arbitrary versus arbitrary). If you are okay with these changes, then we should be set to merge. Thanks for your help getting this implemented!

Member

ethan-nelson commented Aug 8, 2018

Hi Pierre, I added a database revision to handle the case where a database has been migrated prior to your changes. In addition, I also added to the migration script the case where zoom values were not none so that projects would not have a blank value.

I ran the revision file and it worked for a local clone (the ratio was about 9 to 1 for non-arbitrary versus arbitrary). If you are okay with these changes, then we should be set to merge. Thanks for your help getting this implemented!

out of date

@bgirardot

This comment has been minimized.

Show comment
Hide comment
@bgirardot

bgirardot Aug 8, 2018

Collaborator

I also got a little closer to getting a file to test this with. will update in a few days with the file hopefully.

Collaborator

bgirardot commented Aug 8, 2018

I also got a little closer to getting a file to test this with. will update in a few days with the file hopefully.

@ethan-nelson

This comment has been minimized.

Show comment
Hide comment
@ethan-nelson

ethan-nelson Aug 10, 2018

Member

Thanks Blake. I was able to generate a test one using Pierre's test that worked and I was able to confirm the TM populated those fields.

Member

ethan-nelson commented Aug 10, 2018

Thanks Blake. I was able to generate a test one using Pierre's test that worked and I was able to confirm the TM populated those fields.

@ethan-nelson ethan-nelson merged commit f1eeadb into hotosm:develop Aug 10, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@dakotabenjamin dakotabenjamin referenced this pull request Aug 22, 2018

Merged

Release v3.0.2 #1169

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