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

Enhancement: Support tags or some other filtering mechanism #75

Closed
archaeogeek opened this issue Dec 14, 2016 · 20 comments
Closed

Enhancement: Support tags or some other filtering mechanism #75

archaeogeek opened this issue Dec 14, 2016 · 20 comments
Assignees

Comments

@archaeogeek
Copy link
Contributor

I'd like to be able to define tags for entries and then be able to filter by those tags. An entry might have more than one tag. Either tags could be free text, entered by the user when they create an entry, or defined by the admin and then picked when creating an entry. It should then be possible to filter by those tags in the left-hand menu or using the search box.

@justb4
Copy link
Member

justb4 commented Dec 15, 2016

Good suggestion! I see a relation with #18. I have been thinking in similar lines: one possibility using key/value pairs, ala OSM. But plain tags may be simpler to implement and could possibly tackle both #18 and #75.

@samperd
Copy link

samperd commented Feb 8, 2017

I am OK with moving forward with "tags" that seems pretty flexible. Currently we have little need for access controls. We try to be open by default :)

tomkralidis added a commit to tomkralidis/GeoHealthCheck that referenced this issue Feb 8, 2017
tomkralidis added a commit to tomkralidis/GeoHealthCheck that referenced this issue Feb 8, 2017
tomkralidis added a commit to tomkralidis/GeoHealthCheck that referenced this issue Feb 8, 2017
tomkralidis added a commit to tomkralidis/GeoHealthCheck that referenced this issue Feb 8, 2017
@tomkralidis
Copy link
Member

First pass implemented in master (#83). Feedback / testing would be much appreciated :)

@samperd
Copy link

samperd commented Feb 8, 2017

@tomkralidis I tried an upgrade of my quite old GHC to test this out. I am now getting DB errors I think. Check out related issue (#84).

@justb4
Copy link
Member

justb4 commented Feb 8, 2017

I also upgraded. Found no issues upgrading, using an existing instance (with PostgreSQL) and config_site.py of the master branch. This is what I did:

  • pulled latest GitHub
  • paver setup (probably not required)
  • paver create
  • run python GeoHealthCheck/app.py

This created the new tables tag and resource_tags but kept my existing tables resource, run etc intact. That is good! Running python GeoHealthCheck/models.py run ok.

In the GUI, what worked:

  • on the left the new filter with tags is shown
  • I could add a Resource and add tags to it
  • I see the tag and resource_tags records added to the DB
  • I see the new tags in the GUI tag filter section

What did not work:

  • clicking on a tag went to http://localhost:8000/?lang=en&tag=mytag but still showed all Resources
  • in edit Resource: adding tags to existing or new (that already had tags) Resources
  • in edit Resource: removing tags

A good start!

@justb4
Copy link
Member

justb4 commented Feb 8, 2017

To add: add.html and resource.html could show a select.. option (combobox) input to select from all existing tags. Also resource.html has
<input type="text" value="{{ resource.tags2csv }}" data-role="tagsinput" disabled/> which does not allow adding new tags by means of the disabled keyword.

@justb4
Copy link
Member

justb4 commented Feb 8, 2017

As for clicking on a tag showing all Resources, I think https://github.com/geopython/GeoHealthCheck/blob/master/GeoHealthCheck/views.py#L65

    if tag is not None:
        response['resources'] = models.Resource.query.filter(
            models.Tag.name.in_([tag])).all()

Returns all Resources, so this query may need adaptation.

@justb4
Copy link
Member

justb4 commented Feb 9, 2017

Suggestion: on https://github.com/geopython/GeoHealthCheck/blob/master/GeoHealthCheck/models.py#L73
add autoincremented identifier, like:

resource_tags = DB.Table('resource_tags',
DB.Column('identifier', DB.Integer, primary_key=True, autoincrement=True),
                         DB.Column('tag_id', DB.Integer,
                                   DB.ForeignKey('tag.id')),
                         DB.Column('resource_identifier', DB.Integer,
                                   DB.ForeignKey('resource.identifier')))

At least in PostgreSQL this makes the table manageable in tools like phpPGAdmin.

@tomkralidis
Copy link
Member

Thanks for the feedback! Added the following fixes:

  • homepage query by tag fixed
  • edit resource tags implemented
  • model identifier changes implemented

Todo:

  • select via autocomplete from existing tags
  • fix tag counts

@justb4
Copy link
Member

justb4 commented Feb 9, 2017

Yes I can edit (add, remove) tags, even for existing Resources that had no tags. Two enhancements proposed:

  • the Edit, Cancel, Save buttons are next to the Name field, they would be better below the Tags entry
  • the Tags input field can be wider

@tomkralidis
Copy link
Member

@justb4 thanks for the feedback. I've moved the Edit, Cancel, Save buttons into their own row in the Resource page. Where should the tags input field be wider? On the resource info page or the add resource page? Or both (changes welcome)?

@justb4
Copy link
Member

justb4 commented Feb 12, 2017

@tomkralidis ok, good for the buttons, the tags input field is ok: only 2 enhancements directly seen:

  • when editing tags in resource.html, no suggestions for existing tags
  • after edit/refresh main dashboard, the tag counts in the tag-filter menu left are not updated

@justb4
Copy link
Member

justb4 commented Feb 12, 2017

@tomkralidis working reviving/expanding the tests in tests dir, I found the following improvements for the models.Tag table:

  • allow creation programmatically (e.g. from fixtures file)
  • set constraints non-nullable and unique on name field (similar to User.email)

Results in this:

class Tag(DB.Model):
    id = DB.Column(DB.Integer, primary_key=True)
    name = DB.Column(DB.String(100), unique=True, nullable=False)

    def __init__(self, name):
        self.name = name

@tomkralidis
Copy link
Member

Thanks @justb4; fixed. master branch should now address the tagging functionality (suggestions, counts, editing, etc.). If someone can test that would be great.

@justb4
Copy link
Member

justb4 commented Feb 20, 2017

Tested, handling much smoother with <option> elements!, issues:

  • adding tags on creating resources: adding an existing and new tag does not create the new tag
  • removing a tag in Edit (sometimes) gives a duplicate key error in /resource/<N>/update and app needs restart
IntegrityError: (raised as a result of Query-invoked autoflush; consider using a session.no_autoflush block if this flush is occurring prematurely) (psycopg2.IntegrityError) duplicate key value violates unique constraint "tag_name_key"
DETAIL:  Key (name)=(tag2) already exists.
 [SQL: 'INSERT INTO tag (name) VALUES (%(name)s) RETURNING tag.id'] [parameters: {'name': u'tag2'}]

Last error is hard to reproduce, several times seen. First the update fails and later I get the error screen.

@tomkralidis
Copy link
Member

Thanks @justb4. first issue fixed, I cannot reproduce the second issue.

@justb4
Copy link
Member

justb4 commented Feb 21, 2017

I'll look into second, after merging master into https://github.com/justb4/GeoHealthCheck/tree/plugins_82. As the Edit-part of resource.html is now in edit_resource.html, also moved the JQuery select2, appeared to be handy for Probe/Check editing as well! NB note that JQuery POST request should use error (next to success, handler not failure to catch errors.

@archaeogeek
Copy link
Contributor Author

I tried to do an in-place upgrade to the latest version to include this functionality, and failed on the paver create step. In the database it created a tags table but not resource_tags, unfortunately I got no error, it just sat at the "Creating database objects" step for an hour. I got around this by creating a new database, editing config_site.py, running paver create and then restoring the old database over the top. So far so good, though I haven't done much tag creation yet...

@justb4
Copy link
Member

justb4 commented Feb 28, 2017

My guess: you are using PostGIS. I found some hanging issues exclusively with PG, never SQLite. Found that SQLAlchemy is in cases aggressively locking PG. Explicitly closing SQLA Sessions (after commit()) helped. This is for the GHC code to be fixed. In some cases it happened when GHC Flask web app was running at the same time as the upgrade. Stopping GHC helped. But looks like you found a workaround.

tomkralidis pushed a commit that referenced this issue Mar 18, 2017
archaeogeek pushed a commit to archaeogeek/GeoHealthCheck that referenced this issue Mar 29, 2017
@justb4
Copy link
Member

justb4 commented May 3, 2017

Tag-support is done, now in maintenance mode. Locking issue also solved with code that went in via #82 (PR #93). See also demo on http://demo.geohealthcheck.org

@justb4 justb4 closed this as completed May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants