theodi / open-data-certificate

The mark of quality and trust for open data
https://certificates.theodi.org/
MIT License
46 stars 39 forks source link

Add Poke to Question ID #1683

Closed Stephen-Gates closed 6 years ago

Stephen-Gates commented 6 years ago

After experimenting in development, the only way I can see to make the

Changing some question ids breaks reading CKAN e.g. publisherRights- see comments in XML.

Bug #1391 persists and presents a bad user experience. It should be fixed seperately for all certificate users.

Also removing references to theodi.org.au help until the help is finalised.

From development:

CKAN read

screenshot 2018-06-18 06 16 32

Wording changes reflected

screenshot 2018-06-18 06 27 21

Certificate awarded (and warning that the survey changed)

screenshot 2018-06-18 06 18 41

Certificate published

screenshot 2018-06-18 06 20 05

Campaign run

screenshot 2018-06-18 06 23 14

Certificates automatically awarded

screenshot 2018-06-18 06 24 12

cc: @olivierthereaux @ldodds @mattRedBox

Stephen-Gates commented 6 years ago

Sorry I am a GitHub novice. I'm not sure how to fix the Travis complaint.

olivierthereaux commented 6 years ago

Hi @Stephen-Gates just had a look at this. The Travis CI error does not worry me too much - following the link seems to point to some odd git object index corruption but job eventually completes.

That said, it would be better if we could avoid this POKE kludge. How did you figure out that the question identifiers needed to change?

I wonder if @pezholio ran into this issue in the past - whereby a new localisation could not be successfully installed. Stuart, anything you can rememeber and/or any pointer you might have?

Stephen-Gates commented 6 years ago

Hi @olivierthereaux I agree it is a hack and I'd love to avoid it. I noticed that the same error of not updating the words exists in production with the GB Final survey

screenshot 2018-06-17 09 12 01

Changing the question id was just an idea to try trigger a change and it worked. This came from the new dataBreach question working fine but others with unchanged question ids not updating. There must be a more correct way to tell the user interface to update itself.

Stephen-Gates commented 6 years ago

I forgot to mention that when I removed "Poke" from the question id, the original

That suggests to me something is being stored/cached somewhere.

olivierthereaux commented 6 years ago

@Stephen-Gates I believe the questions are being stored in the database, yes. Hence the rake tasks, as @mattredbox mentioned.

pezholio commented 6 years ago

Hmmmm... it's lost to the annals of time for me I'm afraid. I don't recall having any of these issues though. It might be worth looking at the database and seeing if the IDs have any uniqueness enforced maybe?

Stephen-Gates commented 6 years ago

Thanks @pezholio @olivierthereaux what do you suggest as the next step?

olivierthereaux commented 6 years ago

No magical solution at this very moment @Stephen-Gates but it would be good to open an issue documenting the fact that changes to the questions does not get reflected in the service. Can you do so, and can anyone in your team start investigating?

We may have time in ODI HQ's tech team to look into it, but not before end of week. Meanwhile I'm progressing paperwork on contributor agreements as fast as I can, so your team can at least deploy and test to staging, but given that the issues appear to be consistent with what you had locally, it's not in immediate critical path.

olivierthereaux commented 6 years ago

FWIW, I've resolved the Travis conflict by updating the branch. Would be good to make sure to re-synch ODIQueensland/open-data-certificate with upstream.

Stephen-Gates commented 6 years ago

Thanks @olivierthereaux So will you consider merging and running the build commands for this PR or is the "Poke" hack a show-stopper?

olivierthereaux commented 6 years ago

Am clearly not a fan of the hack, but for now mostly curious what you meant by

Changing some question ids breaks reading CKAN e.g. publisherRights- see comments in XML

Stephen-Gates commented 6 years ago

When I changed the publisherRights question to try force a wording change, in the survey:

If I reverted the changes:

I assume some question ids have special meaning and are used elsewhere in the code.

I don't believe CKAN metadata answers any of the Privacy questions so the hack in the Privacy is ok.

I think accepting the PR in staging is low risk as I don't believe it would impact other jurisdictions.

We're exploring getting a developer to look at the root cause but this will probably not meet our timeline given that we may need to get both code and xml thru staging and production.

olivierthereaux commented 6 years ago

I see, thanks @Stephen-Gates. I'm hoping this doesn't cascade into more pain, but I'm happy going ahead with this workaround. Do you think your team can generate the updated .rb and .yml files, as previously? I will then take care of the deployment and rake on the server.

ghost commented 6 years ago

Hi @Stephen-Gates @olivierthereaux .rb and .yaml merged from your recent updates @Stephen-Gates -pending CI checks

olivierthereaux commented 6 years ago

CI passed! I'm merging and deploying to Staging now.

Stephen-Gates commented 6 years ago

Thanks @olivierthereaux. Testing in Staging

CKAN read

screenshot 2018-06-21 06 38 07

Wording changed

screenshot 2018-06-21 06 41 19

Certificate awarded & warning that survey changed

screenshot 2018-06-21 06 43 16

Certificate published

screenshot 2018-06-21 06 49 23

Certificate can be viewed

screenshot 2018-06-21 06 49 50

So looking good. Will test campaigns next.

Stephen-Gates commented 6 years ago

Campaign testing...

Create a campaign

screenshot 2018-06-21 06 59 15

Start a campaign

screenshot 2018-06-21 07 00 11

Certificates awarded

screenshot 2018-06-21 07 02 44

View an automatically awarded certificate

screenshot 2018-06-21 07 04 31

So at a course level of testing it works!!!

I'll test more thoroughly tonight, I'm away at an off-site event today.

Stephen-Gates commented 6 years ago

Same results in staging and prod for other jurisdictions (USA)

Staging

screenshot 2018-06-21 18 44 18

Production

screenshot 2018-06-21 18 43 43
Stephen-Gates commented 6 years ago

Line of questioning is correct but #1391 provides a bad user experience

screenshot 2018-06-21 18 50 54
Stephen-Gates commented 6 years ago

@olivierthereaux this change works as expected and doesn't seem to break other jurisdictions. Bug #1391 is annoying but has been there since early 2016.

I hope the above screenshots are sufficient evidence to give you confidence to deploy the certificate to Production.

Are you able change the status to Final and deploy to Production? Then I'll run a campaign.

olivierthereaux commented 6 years ago

Looks good, thank for thorough testing. I am afk today so won't get to deploy to production before late afternoon, possibly early tomorrow.

Stephen-Gates commented 6 years ago

Great thanks @olivierthereaux