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

More readable & detailed Changes page #4929

Merged
merged 46 commits into from
Sep 5, 2019

Conversation

hayley-leblanc
Copy link
Contributor

@hayley-leblanc hayley-leblanc commented Jul 26, 2019

The current "Changes" page shows a JSON diff between two consecutive versions of a dataset, which is difficult to read and not particularly user-friendly. This PR provides a much more readable and detailed summary of each change on the "Changes" page. The JSON diff is still viewable for each change, but it is not automatically displayed. This PR also adds the ability to view the changes that were made in multiple consecutive revisions of a dataset (see image below).

change_page

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Adds a more readable summary of changes between two versions of a dataset (only for dataset metadata fields - title, description, license, etc. and only for those that have actually changed) to the Changes page. The comprehensive but less readable metadata diff is still there but is opened with a button and not shown by default. Does not yet show a summary for custom fields or fields that come with extensions - only shows changes for the default fields.
@hayley-leblanc
Copy link
Contributor Author

A couple of new tests fail because in order to make the Changes page more useful, I removed the lines preventing activity about private datasets from being saved.

@hayley-leblanc hayley-leblanc deleted the version-control branch July 30, 2019 11:52
@hayley-leblanc hayley-leblanc restored the version-control branch July 30, 2019 11:53
@amercader
Copy link
Member

@hayley-leblanc Thanks for this, really exciting

@davidread you seem like the best placed to lead on the review of this

Copy link
Contributor

@davidread davidread left a comment

Choose a reason for hiding this comment

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

Great to see this! This really enhances the Changes page, making it much easier for users to view this info.

I've reviewed most of this and the templating/i18n in changes.py needs looking at the most. Once this is resolved I just want to look through the detail of changes.py to check any edge cases.

ckan/lib/activity_streams_session_extension.py Outdated Show resolved Hide resolved
ckan/lib/changes.py Outdated Show resolved Hide resolved
ckan/lib/helpers.py Show resolved Hide resolved
ckan/templates/package/changes.html Outdated Show resolved Hide resolved
ckan/templates/package/snippets/change_item.html Outdated Show resolved Hide resolved
ckan/templates/snippets/activities/changed_package.html Outdated Show resolved Hide resolved
ckan/lib/helpers.py Outdated Show resolved Hide resolved
davidread pushed a commit that referenced this pull request Aug 10, 2019
5 tests fail because they need additions to changes.py:
* add & remove of owner_org
* add/change/remove of resource_extras

I've included a couple of other little improvements to changes.py too.
hayley-leblanc pushed a commit to SFB-ELAINE/ckan that referenced this pull request Aug 13, 2019
5 tests fail because they need additions to changes.py:
* add & remove of owner_org
* add/change/remove of resource_extras

I've included a couple of other little improvements to changes.py too.
@hayley-leblanc
Copy link
Contributor Author

All the tests pass now, except for one (test_remove_org), which I cannot for the life of me get to pass. I can't replicate the issue either; the same case works just fine in my instance of CKAN.

@smotornyuk
Copy link
Member

smotornyuk commented Aug 27, 2019

I was able to reproduce fail locally and noticed that the one, that unsets dataset organization is failing. update_package_schema has ignore_missing as first validator for owner_org and corresponding action just assumes that you don't want to update the organisation looking at its new value. Something, like direct model change would work:

    @helpers.change_config(u'ckan.auth.create_unowned_dataset', True)
    def test_remove_org(self):
        changes = []
        old_org = Organization()
        original = Dataset(owner_org=old_org['id'])

        import ckan.model as model
        pkg = model.Package.get(original['id'])
        pkg.owner_org = None
        pkg.save()

        new = helpers.call_action(
            u'package_show', id=original['id'])

        check_metadata_changes(changes, original, new)

        eq(changes[0]['type'], u'org')
        eq(changes[0]['method'], u'remove')

but if there are no particular reason, why you put owner_org here, it would be easier to just test another field.

@davidread
Copy link
Contributor

Many thanks @smotornyuk ! I've added that test fix and opened a fresh PR to merge this (because that is simpler than asking @hayley-leblanc to add it to this one): #4969

@davidread davidread merged commit f9e538e into ckan:master Sep 5, 2019
@davidread
Copy link
Contributor

This is merged now 🎉 @hayley-leblanc many thanks for doing the hard work to meet all our CKAN standards - this is a really valuable contribution 👍

@amercader
Copy link
Member

Thanks @hayley-leblanc! Really exciting to see such a valuable contribution. And thanks @davidread for a proactive and thorough review

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.

None yet

6 participants