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

initial implementation of bona-fide-bot #2456

Merged
merged 11 commits into from
Dec 1, 2020
Merged

initial implementation of bona-fide-bot #2456

merged 11 commits into from
Dec 1, 2020

Conversation

opqdonut
Copy link
Contributor

@opqdonut opqdonut commented Nov 24, 2020

for #1523

Definition of Done / Review checklist

Reviewability

  • link to issue
  • consider adding screenshots for ease of review

Documentation

  • update changelog if necessary
  • add or update docstrings for namespaces and functions
  • documentation at least for config options (i.e. docs folder)
  • ADR for major architectural decisions or experiments

Testing

  • complex logic is unit tested
  • valuable features are integration / browser / acceptance tested automatically

Follow-up

  • new tasks are created for pending or remaining tasks
  • no critical TODOs left to implement

@opqdonut opqdonut changed the title WIP: initial implementation of bonafide-bot initial implementation of bonafide-bot Nov 25, 2020
Copy link
Collaborator

@Macroz Macroz left a comment

Choose a reason for hiding this comment

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

Some ideas for discussion. Looks good!


(def bot-userid "bonafide-bot")

(defn- find-email-address [application]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add some protection about having just one email address? Or just use find-first to be more explicit about which to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could be an assertion too I guess, but it's a bit impolite if it fails only when the bot tries to do something, not when creating the workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah ideally there would be some collaboration with the form editor or you would pick the email field in the workflow/bot settings. Or this could be a report of its own, data validation sort of things. We don't have to do it now as long as the docs match the exact behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use field ids for this btw, fetching the unique field with id "referrer-email" or something?

Of course if there are multiple forms there might be multiple fields with the same field id, but we could use the first form.

test/clj/rems/api/test_end_to_end.clj Outdated Show resolved Hide resolved
src/clj/rems/application/bonafide_bot.clj Outdated Show resolved Hide resolved
docs/bots.md Outdated Show resolved Hide resolved
docs/bots.md Outdated Show resolved Hide resolved
docs/bots.md Outdated Show resolved Hide resolved
@opqdonut opqdonut changed the title initial implementation of bonafide-bot initial implementation of bona-fide-bot Dec 1, 2020
@Macroz Macroz merged commit 1c3d7aa into master Dec 1, 2020
@Macroz Macroz deleted the bonafide-bot-1523 branch December 1, 2020 17:08
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.

2 participants