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

Polishing Changes to the Config Automation #49

Merged
merged 9 commits into from
Dec 10, 2023

Conversation

Abby-Wheelis
Copy link
Member

@Abby-Wheelis Abby-Wheelis commented Nov 20, 2023

Changes to polish the config automation including:

  • update commits to only include /configs files
  • adding link to reference the action I based mine on
  • ensuring the LISENCE is correct
  • potential work on language handling
  • example trip survey that's in e-mission (currently in @sebastianbarry's repo)

(tasks from review of #46)

Abby-Wheelis and others added 3 commits November 20, 2023 11:12
@Abby-Wheelis
Copy link
Member Author

ensuring the LISENCE is correct

The repo that I based the issue -> JSON converted on does not have a license, at least at this moment, so my plan is to make sure that I link to the files I was adapting to make sure credit is still provided.

@Abby-Wheelis
Copy link
Member Author

In d39aa9a and b3e217f I copied the file from @sebastianbarry's repo into this PR, and updated the link to reflect where that sample file will rest in the e-mission repo once this is merged.

The thing I don't have is the xml or xls files for that survey, which might be easier to read if someone was trying to reference them. @sebastianbarry (and maybe @JGreenlee who also had commits in the repo where I found the survey), do either of you happen to still have either of those original files? Maybe they had a different name in the repo but I couldn't find them.

@JGreenlee
Copy link
Collaborator

JGreenlee commented Nov 29, 2023

As far as I know, the xml and xls have been missing the whole time. I think that survey was created by external partners – it may be that they only provided it to us in JSON format because the others aren't required for it to work.

@JGreenlee
Copy link
Collaborator

There's some chance @asiripanich would have the original XML or Excel files for the trip-end-survey? It was many months ago so it's understandable if not.

@asiripanich
Copy link
Member

@AlirezaRa94 is

There's some chance @asiripanich would have the original XML or Excel files for the trip-end-survey? It was many months ago so it's understandable if not.

@AlirezaRa94, can you please help @JGreenlee with this? I think it can be easily exported from our Enketo account.

@AlirezaRa94
Copy link
Contributor

@asiripanich
@JGreenlee
Is this the file that you are looking for?
trip-confirm-survey-form.zip

@Abby-Wheelis
Copy link
Member Author

@AlirezaRa94 yes, that matches the JSON file that we have, thank you! If you happen to also have the xlsx file that would be great, but if not no worries!

Abby Wheelis and others added 2 commits December 1, 2023 09:29
@Abby-Wheelis
Copy link
Member Author

I think this is ready for review, unless we want to wait for more changes, but it includes:

  • a fix for the over-comitting of files
  • an additional field for admin dash user emails (supporting the work @nataliejschultz is doing)
  • more comments about sourcing of these files
  • sample files in our repo

Which covers the main goals of this PR. If we later find the xls file for the survey, come up with a better way to handle language, or receive feedback for changes needed from the review and/or testing process that can probably be in a new PR

@Abby-Wheelis Abby-Wheelis marked this pull request as ready for review December 6, 2023 16:08
@Abby-Wheelis
Copy link
Member Author

Screenshot 2023-12-06 at 9 14 51 AM
Screenshot 2023-12-06 at 9 15 33 AM

Some examples of what the new field looks like once the issue has been created, and in the issue form.

id: admin_access
attributes:
label: Admin Access Emails
description: Please enter a comman separated list of emails to which we will grant access to the admin dashboard. This will usually be a relatively short list.
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a limit of 3 users in the appendix. Maybe we can add a check for this number in the validation and limit it to less than 5?

@@ -1,4 +1,5 @@
# this action is based on the action by zachleat on github (github-issue-to-json-file)
# https://github.com/zachleat/github-issue-to-json-file/blob/main/action.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

@Abby-Wheelis in general, we cannot use copy that does not explicity have a LICENSE. We can "adapt" based on it if the changes are sufficiently large. You might want to check with legal on whether this meets that threshold.

@shankari
Copy link
Contributor

Better than before, needs some check/follow-up on the LICENSE

@shankari shankari merged commit a07365a into e-mission:main Dec 10, 2023
Abby-Wheelis pushed a commit to Abby-Wheelis/nrel-openpath-deploy-configs that referenced this pull request Jan 26, 2024
based on review comment - e-mission#49 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Address code reviews with "future"
Development

Successfully merging this pull request may close these issues.

5 participants