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

Retrieve username from request object and not xform object #871

Merged
merged 6 commits into from Jan 23, 2017

Conversation

ivermac
Copy link
Contributor

@ivermac ivermac commented Jan 12, 2017

closes #870

@ukanga
Copy link
Member

ukanga commented Jan 13, 2017

I would have thought the correct Enketo URI is the owner of the form and not the requesting user. This has the likely hood of generating the wrong records in Enketo that may end up clashing with a users own forms. Suppose the user B is accessing a form from user A with the id_string tutorial, User B also has a form with id_string tutorial. Suppose that the two forms have different content, which form will be shown?

ukanga
ukanga previously requested changes Jan 13, 2017
Copy link
Member

@ukanga ukanga left a comment

Choose a reason for hiding this comment

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

This approach needs to be reconsidered.

@ivermac
Copy link
Contributor Author

ivermac commented Jan 15, 2017

You make a good point and I agree with you that this approach needs to be reconsidered but at the same time preview urls are not working for users with can view and download as well as can view permissions - I tested this out after @msschroeder reported and I replicated the issue (<id_string> not found in formlist issue). What I have here fixes the preview url problem as illustrated in [enketo's preview url doc] (https://apidocs.enketo.org/v2#/get-survey-preview).
As a side note, after working on the formbuilder, I think I now understand why kobo decided to go with the approach of generating an id_string for the user; the generated id_string contains random characters which seems to be unique not only in a person's account but the xform table as well. With this, one would never be worried about having duplicate id_strings in a formList even if forms from different accounts were shared with a particular user.
The other thing I had discussed with @royrutto and @denniswambua last week was that I don't see the reason why core/onadata is the middle service when zebra wants an enketo url or enketo preview url. We have forms/<id>/enketo endpoint which returns both an enketo url as well as an enketo preview url. For example, the request format for getting a preview url is the following:
http -a <enketo-api-token>: POST "https://enketo.ona.io/api_v2/survey/preview?server_url=<user-specific-formlist-endpoint>&form_id=<id-string>"
I think, but I stand to be corrected, zebra can acquire all the 3 variables above and make a request to enketo without having core make the request on zebra's behalf.

@pld
Copy link
Member

pld commented Jan 15, 2017 via email

@ivermac
Copy link
Contributor Author

ivermac commented Jan 15, 2017

An example would be https://api.ona.io/pld/formList

@pld
Copy link
Member

pld commented Jan 16, 2017

Cool I would say that yes zebra should definitely do this. It will reduce traffic and new faster. @ivermac can you create a zebra issue for this?

Regardless we should close this core issue so @ukanga please suggest a way forward for that.

@ivermac
Copy link
Contributor Author

ivermac commented Jan 16, 2017

@pld I have the zebra issue

@ukanga
Copy link
Member

ukanga commented Jan 17, 2017

@ivermac what is the reason that those with permissions to the form have no access to the form?
What is the reason for the not found message?

@ivermac ivermac force-pushed the 870-incorrect-preview-url-returned-by-core branch from 9d5714b to 37dc1df Compare January 18, 2017 11:51
@ivermac
Copy link
Contributor Author

ivermac commented Jan 18, 2017

For context, @ukanga and I decided to add new formList endpoint that would be used only for enketo preview urls. This is because at the moment, the formlist endpoint only allows users without can view or can view and download permission to access that endpoint - it's a permission issue which @ukanga can give a better explanation if i'm not being clear.

@msschroeder
Copy link

Is there any update when this issue will be deployed? The customer was asking when this will be resolved. I was under the impression that it would be deployed this week.

@ivermac
Copy link
Contributor Author

ivermac commented Jan 20, 2017

@msschroeder I can't really merge it since I worked on it. @denniswambua or @ukanga should be able to review this PR and give a status update.

@msschroeder
Copy link

@denniswambua What else needs to be done with this? Can you give a timeline for when this will be done?

@denniswambua
Copy link
Contributor

@msschroeder Its ready for QA

denniswambua
denniswambua previously approved these changes Jan 23, 2017
@denniswambua denniswambua dismissed their stale review January 23, 2017 07:39

no documentation

Copy link
Contributor

@denniswambua denniswambua left a comment

Choose a reason for hiding this comment

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

Add documentation

@ivermac ivermac force-pushed the 870-incorrect-preview-url-returned-by-core branch from 37dc1df to 68e649a Compare January 23, 2017 08:03
@ivermac
Copy link
Contributor Author

ivermac commented Jan 23, 2017

@denniswambua I have added documenation

@skambo skambo added the QA+ PR passed QA testing label Jan 23, 2017
@skambo
Copy link
Contributor

skambo commented Jan 23, 2017

Can view and Can view and download users now get Enketo preview URL

@denniswambua denniswambua merged commit f019f6a into master Jan 23, 2017
@denniswambua denniswambua deleted the 870-incorrect-preview-url-returned-by-core branch January 23, 2017 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA+ PR passed QA testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect enketo preview url returned by core
6 participants