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

Remove appended campaign id appended to new campaign name #44

Merged
merged 3 commits into from
Sep 16, 2021

Conversation

nickviola
Copy link
Contributor

@nickviola nickviola commented Aug 24, 2021

Remove appended CX id to campaign name when creating new campaign. We are changing the way we parse exports and will reference by the format "_levelX" instead of CX identifiers. This gives us the ability to correlate levels of assessments with campaigns in GoPhish exports.

πŸ—£ Description

This removes the old structure of appending CX to the campaign name. We no longer need this format and will be referencing by level_id (1-6).

πŸ’­ Motivation and context

We no longer need this format and will be referencing by level_id (1-6).

πŸ§ͺ Testing

To test and validate that the name format change was implemented correctly, I did the following:

  1. Build a new image that includes the changes in this branch and tag it for reference by running:
    docker build -t gophish-tools-issue-43 .

  2. Run the following to override the default image and reference the newly tagged image with the changes:
    export GOPHISH_TOOLS_IMAGE="gophish-tools-issue-43" && eval "$(docker run gophish-tools-issue-43)"

  3. The alias should be setup to drop into a bash instance on the gophish-tools container. You can run the following to access the container:
    gophish-tools-bash

  4. . In the contaner bash instance, create a new assessment as you normally would by running the alias:
    pca-wizard

  5. Input Campaign data and import into gophish. The campaign name should now exclude the appended _C1 that was originally there.

I also confirmed that the campaign went through all phases correctly sending email and generating click output as expected with the new name format.

βœ… Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.

@nickviola nickviola added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Aug 24, 2021
@nickviola nickviola marked this pull request as ready for review September 1, 2021 15:25
@jsf9k
Copy link
Member

jsf9k commented Sep 1, 2021

You should remove the unchecked checkbox under "Checklist" in the PR description, unless you intend to add tests fro this change.

Also, I didn't see any documentation updated here, so I suggest removing the "All relevant repo and/or project documentation..." line as well (unless you intend to add it).

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

This looks good to me. πŸ‘

In the "Testing" section of your PR description, I'd like to see this written as "Here's what I did and how I confirmed that everything is correct", rather than what you currently have which comes across more like "Here's what I plan to test" with no indication of successful testing.

@nickviola
Copy link
Contributor Author

You should remove the unchecked checkbox under "Checklist" in the PR description, unless you intend to add tests fro this change.

Also, I didn't see any documentation updated here, so I suggest removing the "All relevant repo and/or project documentation..." line as well (unless you intend to add it).

Thanks for catching that. I meant to go back and remove them, but my memory failed me. πŸ˜†

I've updated the checklist! Thanks for looking it over!

@nickviola
Copy link
Contributor Author

This looks good to me. πŸ‘

In the "Testing" section of your PR description, I'd like to see this written as "Here's what I did and how I confirmed that everything is correct", rather than what you currently have which comes across more like "Here's what I plan to test" with no indication of successful testing.

πŸ‘ Agreed. That provides more clarity. I've updated it to include more details on how it was tested. Thanks for looking it over!

@nickviola nickviola linked an issue Sep 2, 2021 that may be closed by this pull request
1 task
@nickviola nickviola self-assigned this Sep 2, 2021
@dav3r dav3r merged commit 442cdb5 into develop Sep 16, 2021
@dav3r dav3r deleted the issue-43/remove-auto-appended-campaign-id branch September 16, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Auto Appended Campaign Identifier During Creation
4 participants