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

Exportcert command updates [mid v1.4] #222

Merged
merged 20 commits into from
Nov 6, 2018

Conversation

sharan8
Copy link

@sharan8 sharan8 commented Nov 4, 2018

This pull request closely follows the improvements outlined in #175.

The following updates have been implemented:

  • Refactor to use predicate classes EventContainsEventIdPredicate and RecordContainsVolunteerIdPredicate for filtering through the event records and events.
  • Exported certificate design is updated with better text alignments, text sizes and templated text.
  • Tests are updated to reflect exporting for volunteers with records only.
  • Support for absent event records is abstracted away from createPdf method, and is now placed earlier, in the execute method itself.
  • Added TypicalVolunteersWithRecords.java to be able to run tests on a model which has both volunteers as well as records. (Note: exportcert only works on volunteers who have records)
  • Updated tests to use both model with records and model without records for testing
  • Hardcoded separator '/' replaced with File.separator to be OS agnostic.
  • Certificate export folder renamed to 'Certs'
  • 'INDEX' added to 'Certificate exported for volunteer at INDEX ...' to be more user friendly

Area(s) of Concern:

  • Could the inclusion of addRecordForVolunteer() in CommandTestUtil to add an event record for the first volunteer be improved?

@sharan8 sharan8 added the status.Ongoing The issue is currently being worked on. note: remove this label before closing an issue. label Nov 4, 2018
@sharan8 sharan8 added this to the v1.4 milestone Nov 4, 2018
@coveralls
Copy link

coveralls commented Nov 5, 2018

Coverage Status

Coverage increased (+0.8%) to 71.594% when pulling db6a0e9 on sharan8:exportcert-command into f77c7fd on CS2103-AY1819S1-W16-2:master.

@iMarbles
Copy link

iMarbles commented Nov 5, 2018

Remember to change tag to 'Ready' once you're done!

@sharan8 sharan8 changed the title Exportcert command updates Exportcert command updates [mid v1.4] Nov 6, 2018
@sharan8 sharan8 added status.Ready The PR is ready to be reviewed. note: remove this label before merging a PR. and removed status.Ongoing The issue is currently being worked on. note: remove this label before closing an issue. labels Nov 6, 2018
@iMarbles iMarbles merged commit 05f7774 into CS2103-AY1819S1-W16-2:master Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status.Ready The PR is ready to be reviewed. note: remove this label before merging a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants