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

Fix improper survey metadata generation when using booleans #64

Merged
merged 20 commits into from
Feb 27, 2024

Conversation

VirajP1002
Copy link
Contributor

@VirajP1002 VirajP1002 commented Jan 8, 2024

What is the context of this PR?

Previously test_metadata_routing.json was failing to open because flag_1 (now changed to boolean_flag) wasn't being included within survey_metadata however it was included as top level metadata leading.

In order to fix this issue, the logic in getSurveyMetadataFromClaims was flipped where metadata in claimValues that is not top level metadata is considered to be survey metadata.

claimValues used in getSurveyMetadataFromClaims did not contain boolean_flag when unticked in launcher (set to False) and so the logic where boolean_flag is added back to survey_metadata for token generation in GenerateTokenFromPost had to be changed as the previous implementation added boolean_flag into top level rather then in survey_metadata.

launch.html was updated to ensure that the form submission doesn't contain unwanted metadata.

How to review

  • Ensure the changes made are suitable

  • Run Launcher and Runner locally and ensure test_metadata_routing.json opens when boolean_flag is set to True/False in the Launcher UI.

  • Ensure boolean_flag is not put in top level metadata, do this by adding a breakpoint to line 110 in sessions.py in Runner, open launcher and open test_metadata_routing.json. In the debug window, select decrypted_token and check that boolean_flag is within survey_metadata and is the correct value.

  • Check the loaded values on the launcher UI are correct, do this by adding metadata to a schema such as { "name": "test", "type": "date" }. Load up the launcher UI and check that it has been added in the Survey Metadata section with a value in the field. The same can be checked for altering the type to string,uuid or url. If you run Runner in debug mode, you can check the token to ensure test in within survey_metadata.

  • Check test_metadata_routing opens in quick-launch, add an additional piece of metadata (similar to the one above) to the schema and run Runner in debug mode. Enter the url into the browser and check that the survey_metadata is correct, it should match the one from the UI. You can also add additional arguments to the URL or override an existing piece of metadata and ensure the survey_metadata section is correct.

  • Ensure changes do not break other schemas, e.g. test_supplementary_data, or any business/social surveys

@VirajP1002 VirajP1002 changed the title Improper survey metadata generation Fix improper survey metadata generation Jan 8, 2024
@VirajP1002
Copy link
Contributor Author

Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

Looks good so far, but as per the ticket we need to implement this part as well: If a key type is unknown, and therefore launcher doesn’t have a default for it, use the type of the metadata value to auto-generate something suitable. At the moment, for strings Launcher is sending an empty string e.g. "", but if you added a new metadata with a type like date to metadata in your test schema, the launch would fail as there would be no value set (as no default exists). So we need to dynamically generate values based on the type

@VirajP1002 VirajP1002 changed the title Fix improper survey metadata generation Fix improper survey metadata generation when using booleans Jan 11, 2024
…y metadata and that amending metadata values in the url works as intended and refactor method names
…bmission and update launch.go to remove launch_version from r.PostForm as if it remains it will included as survey metadata
…ch.go to remove the need to delete launch_version from r.PostForm
@MebinAbraham
Copy link
Contributor

MebinAbraham commented Feb 1, 2024

Not directed at your changes, but this highlights how much of a mess the current Go logic is. Unnecessary complexity and confusing logic in quite a few places. Even though it is a dev tool, it might be worth us doing a refactor.

@VirajP1002
Copy link
Contributor Author

Not directed at your changes, but this highlights how much of a mess the current Go logic is. Unnecessary complexity and confusing logic in quite a few places. Even though it is a dev tool, it might be worth us doing a refactor.

I would say it's quite messy by the fact it's hard to navigate, for example going to method calls and definitions is odd as it jumps around the page and that some v1 methods are still there.

Copy link
Contributor

@petechd petechd left a comment

Choose a reason for hiding this comment

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

All the issues above seems to be resolved and all the functionality (scenarios) mentioned work as expected, code is hard to follow but not because of PR creator's design.

@VirajP1002 VirajP1002 merged commit f84ad76 into main Feb 27, 2024
2 checks passed
@VirajP1002 VirajP1002 deleted the improper-survey-metadata-generation branch February 27, 2024 16:05
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.

4 participants