-
Notifications
You must be signed in to change notification settings - Fork 9
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
Highlight candidate application edits on manage (with activity log fix) #8706
Highlight candidate application edits on manage (with activity log fix) #8706
Conversation
4a71de2
to
5af3a24
Compare
5af3a24
to
359cc44
Compare
359cc44
to
9a1b9f8
Compare
9a1b9f8
to
2bbb8b7
Compare
2bbb8b7
to
680aaff
Compare
f03248f
to
b0c0d30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work. ✌️ Let me know your thoughts ✌️
user = event.user_full_name | ||
candidate = event.candidate_full_name | ||
|
||
case event.application_status_at_event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do something similar like:
t("provider_interface.activity_log.application_choice.#{event_name}")
And then on event_name we could do:
def event_name
return event.application_status_at_event if event.application_status_at_event.in?(application_status_list)
# changes attribute ...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I've implemented it the way you described but I've made it less painful to look at and hopefully easier to understand. Translations included
app/components/provider_interface/activity_log/application_form.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Those anchor links to the right section are pure class! :)
This reverts commit eff059b.
Mapping accepts a list of sections and returns the relevant database columns for `.by_section' Mapping accepts a list of database columns and returns a list of corresponding sections for `.by_columns`
Some Audits cannot store a reference to an application choice. We must return the application choice id from the join when we query. When the query returns Audits for ApplicationForm, we only want to return those Audits whose audit_changes are a subset of a whitelist of columns. The app is only tracking audits where personal_information, contact_information, disability_disclosure and interview_preferences are changed.
Polymorphise `ActivityLog#event_description` to handle all possible Autids
b0c0d30
to
c96e21d
Compare
c96e21d
to
b741cbe
Compare
42f441d
to
134465a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✌️ 🚀
Context
Un reverts #8695
Additional changes can be inspected from b0b2e61
ActivityLog in the provider interface collates all audits on applications for the current provider. This query requires access to the application choice. Audits that are created because an application form changes do not have a direct relationship to an application choice so the view logic has no way of generating a link to the relevant application choice.
Screenshots
Changes proposed in this pull request
Here we update the
ActivityLog
query to return theapplication_choice_id
column. We can access this when theAudit#auditable
orAudit#associated
is not anApplicationChoice
Also added is
ApplicationForm::ColumnSectionMapping
.This provides a simple API for getting the columns belonging to a section, and getting the section that a column belongs to.
Guidance to review
Have I collected all the correct columns corresponding to the sections?
Link to Trello card
Trello Ticket