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.

Sign up for GitHub

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

Jump to bottom

Updated readme and added demographics table #63

Merged
merged 8 commits into from
Sep 12, 2023
Merged

Updated readme and added demographics table #63

merged 8 commits into from
Sep 12, 2023

Conversation

achasmita
Copy link
Contributor

No description provided.

@achasmita achasmita changed the title Demographics table Updated readme and added demographics table Aug 31, 2023
@achasmita achasmita mentioned this pull request Aug 31, 2023
Closed
shankari
shankari requested changes Sep 1, 2023
View reviewed changes
docker-compose-dev.yml Outdated Show resolved Hide resolved
utils/constants.py Outdated Show resolved Hide resolved
utils/db_utils.py Outdated Show resolved Hide resolved
utils/permissions.py Outdated Show resolved Hide resolved
pages/data.py Outdated Show resolved Hide resolved
app_sidebar_collapsible.py Outdated Show resolved Hide resolved
docker/start.sh Outdated Show resolved Hide resolved
shankari
shankari requested changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

Almost there!

utils/db_utils.py Outdated Show resolved Hide resolved
docker-compose-dev.yml Outdated Show resolved Hide resolved
pages/data.py Show resolved Hide resolved
shankari
shankari requested changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@shankari shankari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Almost there.
Just some documentation/formatting cleanup and one clarification request...

Please also indicate "Testing done".
"If it is not tested, it is broken"

docker-compose-dev.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
pages/data.py Outdated Show resolved Hide resolved
@shankari
Copy link
Contributor

@achasmita

Code looks fine.
Please indicate "Testing done" before I can merge.
"If it is not tested, it is broken"

@achasmita
Copy link
Contributor Author

@achasmita

Code looks fine. Please indicate "Testing done" before I can merge. "If it is not tested, it is broken"
Screen Shot 2023-09-12 at 11 15 57 AMYes, testing is done and table looks fine.

@shankari
Copy link
Contributor

@achasmita thank you for including the screenshot.
Please do that for all PRs going forward.

However, the testing is not sufficient because it doesn't actually show any of the demographic survey fields.
How do I know if the demographic survey is actually being parsed properly?

Also, please automatically exclude the vast majority of the metadata fields. We don't need the metadata.key (which is the same for all entries, or the metadata.type, ...

Please updated with more descriptive screenshots that show what you are actually testing.

@achasmita
Copy link
Contributor Author

@achasmita thank you for including the screenshot. Please do that for all PRs going forward.

However, the testing is not sufficient because it doesn't actually show any of the demographic survey fields. How do I know if the demographic survey is actually being parsed properly?

Also, please automatically exclude the vast majority of the metadata fields. We don't need the metadata.key (which is the same for all entries, or the metadata.type, ...

Please updated with more descriptive screenshots that show what you are actually testing.

Here is the one after removing metadata.key and including demographic survey fields.Screen Shot 2023-09-12 at 3 32 45 PM

@shankari
Copy link
Contributor

shankari commented Sep 12, 2023 •
edited
Loading

Does this only remove metadata.key? Please note my comment

Also, please automatically exclude the vast majority of the metadata fields.

You can, and should, include multiple screenshots to showcase the breadth of your testing.

@achasmita
Copy link
Contributor Author

Does this only remove metadata.key? Please note my comment

Also, please automatically exclude the vast majority of the metadata fields.

i mean all the columns that starts with metadata.

@achasmita
Copy link
Contributor Author

Here is the screenshot of all the columns it is displaying now:

Screen Shot 2023-09-12 at 3 41 06 PM
Screen Shot 2023-09-12 at 3 42 57 PM
Screen Shot 2023-09-12 at 3 43 22 PM

Screen Shot 2023-09-12 at 3 42 31 PM
Screen Shot 2023-09-12 at 3 43 36 PM

Screen Shot 2023-09-12 at 3 42 06 PM

@shankari
Copy link
Contributor

Please push these changes and I can merge

shankari
shankari reviewed Sep 12, 2023
View reviewed changes
utils/db_utils.py Outdated
df[col] = df[col].apply(str)
columns_to_drop = [col for col in df.columns if col.startswith("metadata")]
df.drop(columns= columns_to_drop, inplace=True)
df.drop(columns=['data.xmlResponse', 'data.name', 'data.version', 'data.label', 'data.jsonDocResponse.aSfdnWs9LE6q8YEF7u9n85.attr.id','data.jsonDocResponse.aSfdnWs9LE6q8YEF7u9n85.attr.xmlns:jr','data.jsonDocResponse.aSfdnWs9LE6q8YEF7u9n85.attr.xmlns:orx'], inplace=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should hardcode any of the form ids (such as aSfdnWs9LE6q8YEF7u9n85) because I believe they can be different from form to form. But the other changes are fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. the form ID for the time use survey, for example, is time_use_survey_form_v9_1
https://github.com/sebastianbarry/nrel-openpath-deploy-configs/blob/surveys-info-and-surveys-data/survey-resources/data-json/time-use-survey-form-v9.json

shankari
shankari approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

I am going to approve this for now and we can tweak based on feedback.

@achasmita
Copy link
Contributor Author

new screenshots Screen Shot 2023-09-12 at 4 26 11 PM Screen Shot 2023-09-12 at 4 25 50 PM

@shankari shankari merged commit dd129b2 into e-mission:master Sep 12, 2023
@shankari shankari mentioned this pull request Sep 15, 2023
Merged
@shankari shankari mentioned this pull request Oct 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shankari shankari shankari approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@achasmita @shankari