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

script to transfer response templates #3549

Merged
merged 1 commit into from Aug 5, 2021
Merged

script to transfer response templates #3549

merged 1 commit into from Aug 5, 2021

Conversation

struan
Copy link
Member

@struan struan commented Jul 28, 2021

Enables transferring of response templates between two installs.

@struan struan force-pushed the issues/freshdesk/1305-transfer-response-templates branch from a1320e4 to bbb1ce2 Compare July 28, 2021 11:02
@struan struan requested a review from davea July 28, 2021 11:02
@codecov
Copy link

codecov bot commented Jul 28, 2021 •
edited

Codecov Report

Merging #3549 (df1855c) into master (34010d3) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head df1855c differs from pull request most recent head a0d65bd. Consider uploading reports for the commit a0d65bd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3549      +/-   ##
==========================================
+ Coverage   82.55%   82.56%   +0.01%     
==========================================
  Files         343      343              
  Lines       23175    23175              
  Branches     3532     3532              
==========================================
+ Hits        19131    19134       +3     
+ Misses       2925     2922       -3     
  Partials     1119     1119
Impacted Files Coverage Δ
web/js/front.js 82.25% <0.00%> (-1.62%) ⬇️
web/cobrands/fixmystreet-uk-councils/alloy.js 91.08% <0.00%> (+3.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34010d3...a0d65bd. Read the comment docs.

davea
davea requested changes Jul 29, 2021
View reviewed changes
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

Not sure if the same(ish) file being present twice is deliberate? Given the strong overlap with export-import-data I think this would be better implemented as a flag (e.g. --update-templates) on that existing script.

@struan struan force-pushed the issues/freshdesk/1305-transfer-response-templates branch from bbb1ce2 to ef89ee8 Compare July 29, 2021 13:32
@struan struan requested a review from davea July 29, 2021 13:33
davea
davea requested changes Jul 29, 2021
View reviewed changes
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

Sorry, I might be missing something but it looks like the flag isn't taken into account on L138 of export-import-data so existing templates are always skipped. Also bin/transfer-updates-response-templates is still hanging around.

@struan struan requested a review from davea July 29, 2021 15:26
davea
davea requested changes Jul 30, 2021
View reviewed changes
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

This is crashing when trying to e.g. reimport a file I just exported:

DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::Pg::st execute failed: ERROR:  duplicate key value violates unique constraint "response_templates_body_id_title_key"
DETAIL:  Key (body_id, title)=(21076, Action Scheduled - Blocked Drain) already exists.
[...]
 at ./bin/export-import-data line 148

@struan struan requested a review from davea August 3, 2021 09:17
davea
davea requested changes Aug 3, 2021
View reviewed changes
bin/export-import-data Outdated
warn "Template with title $_->{title} already exists, skipping";
next;
}
my $template = $body->response_templates->new({
my $template = $body->response_templates->find_or_new({
Copy link
Member

Choose a reason for hiding this comment

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

This won't update the text or status of any existing templates unfortunately. find_or_new will (I believe) only search on unique columns, meaning it'll match on body_id and title then skip over the values for text and state.

Copy link
Member Author

Choose a reason for hiding this comment

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

sigh, yes :(

@struan struan requested a review from davea August 3, 2021 13:58
davea
davea approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

🌟

@struan struan force-pushed the issues/freshdesk/1305-transfer-response-templates branch from 59696d3 to 141453b Compare August 5, 2021 12:37
@struan
update import/export script to optionally update response templates
a0d65bd 
Enables transferring of response templates between two installs.
@struan struan force-pushed the issues/freshdesk/1305-transfer-response-templates branch from 141453b to a0d65bd Compare August 5, 2021 12:37
@struan struan merged commit a0d65bd into master Aug 5, 2021
@github-pages github-pages bot temporarily deployed to github-pages August 5, 2021 12:38 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davea davea davea 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.

None yet
2 participants
@struan @davea