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

CREATE TRANSFORMATION : As an Open Source User, I want to be able to set up a new transformation using my extracted data, so that I can make use of it in the way I need. #4

Merged
merged 9 commits into from
May 4, 2023

Conversation

richardmatthewsdev
Copy link
Contributor

Acceptance Criteria

  • User can create a new transformation
  • User can select extracted data
  • User can specify record selector
  • User can preview output of record selection

Create an extraction
See an Extraction
See a preview record for the extracted data
Add the ability to edit a transformation
Added the abiltiy to delete a transformation
Added the ability to be able to test a transformations record selector
Improve Job selection box so that they are grouped by their respective extraction definition.
Copy link

@isabel-anastasiadis-boost isabel-anastasiadis-boost left a comment

Choose a reason for hiding this comment

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

Would it be good to install codeclimate_diff so that it can be run? I was curious if there were any regressions.

I ran the audit on it, and it returned:

NAME ORGANISATION REPO_HOST RELATIVE_PATH FOLDER MAIN_BRANCH SIMILARITY_SCORE_PER_FILE ABC_METHOD_AVERAGE CODE_SMELLS_PER_FILE TOTAL_SCORE
supplejack_harvester DigitalNZ github.com ../ app create-transformation 2.74 6.1 0.41 9.25

main currently looks like this:

NAME ORGANISATION REPO_HOST RELATIVE_PATH FOLDER SIMILARITY_SCORE_PER_FILE ABC_METHOD_AVERAGE CODE_SMELLS_PER_FILE TOTAL_SCORE
supplejack_harvester DigitalNZ github.com ../ app 1.14 6.1 0.35 7.59

@richardmatthewsdev
Copy link
Contributor Author

Would it be good to install codeclimate_diff so that it can be run? I was curious if there were any regressions

Yeah that's a good idea @isabel-anastasiadis-boost! I'll add it

document.addEventListener('DOMContentLoaded', function () {


function sendData(form) {

Choose a reason for hiding this comment

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

This is quite a big function, would it be easier to read if it was split up a bit? It seems to do more than just send data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is a copy of another test feature that we have. I think we will revisit these when we have a javascript framework in the mix.

Choose a reason for hiding this comment

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

Ahh, it could be that this is one of the code duplication things that was identified too then.

Choose a reason for hiding this comment

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

Could be good to at least not have the duplication if it is mostly a copy paste. Then it is easier to keep improving later


// Transformation Raw Data Record Viewer

const raw_data_record_viewer = document.querySelector("#raw-data-record-viewer");

Choose a reason for hiding this comment

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

This code looks a bit repeated - could it be extracted to a shared function somehow?

@@ -67,4 +67,8 @@ def duration_seconds
def extraction_folder_size_in_bytes
`du #{extraction_folder} | cut -f1`.to_i
end

def name

Choose a reason for hiding this comment

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

Why is the job name a date? Do you want doc comments like the other methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The date is the only identifying information we have at the moment about the job and we show the date it was run on the front end :)

Choose a reason for hiding this comment

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

Is 'name' a misnomer though? Just wondering if it is actually a name or not. And if it is a formatted date, is that view logic, or model logic? Up to you, it just seemed a bit odd to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair :). I actually made this method so that I could get the name for the job in a rails form helper but I am not using that anymore so I will get rid of this method

app/views/content_partners/show.html.erb Show resolved Hide resolved
Copy link

@isabel-anastasiadis-boost isabel-anastasiadis-boost 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 Richard!

Copy link
Contributor

@paul-mesnilgrente paul-mesnilgrente left a comment

Choose a reason for hiding this comment

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

LGTM :) as promised

@richardmatthewsdev richardmatthewsdev merged commit 829daa3 into main May 4, 2023
@richardmatthewsdev richardmatthewsdev deleted the create-transformation branch May 4, 2023 22:55
danielaboost pushed a commit that referenced this pull request May 5, 2024
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