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

Assign multiple authors for a report #3237

Merged
merged 37 commits into from
Nov 18, 2020
Merged

Assign multiple authors for a report #3237

merged 37 commits into from
Nov 18, 2020

Conversation

cemalettin-work
Copy link
Contributor

@cemalettin-work cemalettin-work commented Oct 8, 2020 •
edited
Loading

Reports now have multiple authors. Report form has been changed to accommodate this change. There were two main requirements to satisfy:

  • Reports having multiple authors
  • Some people can be an author but may not attend the engagement

For these requirements we made an enhancement on the form,previously known as "Attendee" section. Form now has "People involved in this engagement" section to cover attending and non-attending people. Authors of the report can cherry-pick who attends, who can author the report from a pool of "people involved". Here is a preview video of the usage.

People in each section are sorted in this order: Primary > Author > Alphabetically.

Release notes

Closes #3215

User changes

  • Users can pick attending people, non-attending people, multiple authors for engagements. With this change, people who will arrange the engagement (like schedulers) can edit the report without being an attendee.

Super User changes

Admin changes

System admin changes

  • anet.yml needs change
  • db needs migration
  • documentation has changed
  • graphql schema has changed

Checklist

  • Described the user behavior in PR body
  • Referenced/updated all related issues
  • commits follow a repo#issue: Title title format and these 7 rules
  • commits have a clean history, otherwise PR may be squash-merged
  • Added and/or updated unit tests
  • Added and/or updated e2e tests
  • Added and/or updated data migrations
  • Updated documentation
  • Resolved all build errors and warnings
  • Opened debt issues for anything not resolved here

@cemalettin-work cemalettin-work changed the title Gh 3215 multiple authors for report Assign multiple authors for a report Oct 8, 2020
@cemalettin-work cemalettin-work requested review from a team and removed request for a team October 9, 2020 09:32
@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2020

This pull request introduces 1 alert when merging a21b2c7 into 8ee8b8c - view on LGTM.com

new alerts:

  • 1 for Useless parameter

@gjvoosten gjvoosten force-pushed the GH-3215-multiple-authors-for-report branch 5 times, most recently from 6fb96cb to efd8bdc Compare October 14, 2020 07:10
@cemalettin-work cemalettin-work force-pushed the GH-3215-multiple-authors-for-report branch 2 times, most recently from 38e7685 to 6a1cfd7 Compare October 14, 2020 14:41
@cemalettin-work cemalettin-work marked this pull request as ready for review October 15, 2020 12:12
@cemalettin-work
Copy link
Contributor Author

I have updated the PR body, and made it ready for reviews.

@cemalettin-work cemalettin-work force-pushed the GH-3215-multiple-authors-for-report branch from f3c8f6e to 828438d Compare October 15, 2020 13:16
@gjvoosten gjvoosten force-pushed the GH-3215-multiple-authors-for-report branch from 828438d to ede976f Compare October 19, 2020 07:20
ysf-simsoft
ysf-simsoft suggested changes Oct 23, 2020
View reviewed changes
client/src/components/advancedSelectWidget/AdvancedSelectOverlayRow.js Outdated Show resolved Hide resolved
client/src/components/ReportTable.js Outdated Show resolved Hide resolved
client/src/pages/reports/ReportPeople.js Show resolved Hide resolved
client/src/models/Report.js Outdated Show resolved Hide resolved
client/src/models/Report.js Outdated Show resolved Hide resolved
client/src/models/Report.js Outdated Show resolved Hide resolved
client/src/models/Report.js Outdated Show resolved Hide resolved
@cemalettin-work cemalettin-work force-pushed the GH-3215-multiple-authors-for-report branch 2 times, most recently from d0dcf31 to 74f0c89 Compare October 26, 2020 08:37
@gjvoosten gjvoosten force-pushed the GH-3215-multiple-authors-for-report branch from a14df56 to f131912 Compare October 29, 2020 09:23
@cemalettin-work cemalettin-work force-pushed the GH-3215-multiple-authors-for-report branch from f131912 to 51b190c Compare November 4, 2020 07:29
gjvoosten and others added 27 commits November 18, 2020 08:56
@gjvoosten
#3215: Show/use multiple report authors
263f7e9
@gjvoosten
#3215: Use report.reportPeople instead of attendees
c152301
@gjvoosten
#3215: Implement FIXME's for e-mail templates
26190ac
@gjvoosten
#3215: Fix client-side tests
40abc19
@gjvoosten
#3215: Fix simulator
3755f4f
@cemalettin-work @gjvoosten
#3215: Rename attendee-author fields to reflect the changes
a612648
@gjvoosten
#3215: Map missing field
7e9ba83
@cemalettin-work @gjvoosten
#3215: Implement report people selection table
927b36e 
Attending non-attending separation

Sort shown people

Administrative section inside the report people section
@cemalettin-work @gjvoosten
#3215: Implement assertions to the report people table
9960123
@gjvoosten
#3215: Add missing condition for updating report people
b16f8bc
@cemalettin-work @gjvoosten
#3215: Change report people table layout
c70ea56
@cemalettin-work @gjvoosten
#3215: Allow current user to change toggle self attendance
e90b483 
Change the heading structures of the report people table
@cemalettin-work @gjvoosten
#3215: Fix failing tests by not depending on the order
f31fbde
@cemalettin-work @gjvoosten
#3215: Change meeting people texts to people involved
804b0f5
@gjvoosten
#3215: Make sure only existing authors can update a report
4d1d38e 
Also check that a report has at least one remaining author.
Add some tests for report authors.
@cemalettin-work @gjvoosten
#3215: Fix report e2e test
00e46f0 
The order was important
@cemalettin-work @gjvoosten
#3215: Prevent same toasts
97dfd40
@gjvoosten
#3215: Make sure reports on the TaskApprovalTest have authors etc.
feadf92
@cemalettin-work @gjvoosten
#3215: Fix weird states non-attending primary report people
3ffd397
@cemalettin-work @gjvoosten
#3215: Refactor/rename
0ad89e4
@cemalettin-work @gjvoosten
#3215: Fix authors name listing
7d02ccd 
Remove unused condition, Add optional chainings for reportPeople
@cemalettin-work @gjvoosten
#3215: Fix style problems of report people table
95e54c0
@cemalettin-work @gjvoosten
#3215: Change administrative to non-attending, also hide when empty
49a3ca2
@cemalettin-work @gjvoosten
#3215: Replace attendee and author checkbox labels with icons
10ff51d
@cemalettin-work @gjvoosten
#3215: Simplify report people table logic
855fe05
@cemalettin-work @gjvoosten
#3215: Remove unnecessary showAuthors prop
31733c4
@gjvoosten
Small readability changes from feedback
d4deb05
@gjvoosten gjvoosten force-pushed the GH-3215-multiple-authors-for-report branch from afc0453 to d4deb05 Compare November 18, 2020 07:58
@VassilIordanov VassilIordanov merged commit 87b7de5 into candidate Nov 18, 2020
@VassilIordanov VassilIordanov deleted the GH-3215-multiple-authors-for-report branch November 18, 2020 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysf-simsoft ysf-simsoft ysf-simsoft approved these changes

@bssimsoft bssimsoft bssimsoft approved these changes

@gjvoosten gjvoosten Awaiting requested review from gjvoosten

@maradragan maradragan Awaiting requested review from maradragan

@VassilIordanov VassilIordanov Awaiting requested review from VassilIordanov

Assignees

@gjvoosten gjvoosten

@VassilIordanov VassilIordanov

@cemalettin-work cemalettin-work

Labels
debt enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiple authors of report
5 participants
@cemalettin-work @gjvoosten @bssimsoft @ysf-simsoft @VassilIordanov