stats4sd / aec_portfolio

A proof of concept for the AEC Consortium Project Management / Assessment System
GNU General Public License v3.0
0 stars 0 forks source link

Add Revision CRUD #193

Closed dan-tang-ssd closed 1 year ago

dan-tang-ssd commented 1 year ago

This PR fixes #148 .

Note: This PR is not fully functioning yet. This is submitted as a progress update.

It provides a Revision CRUD panel. It is accessible via url now. (/admin/revision) I will add it to sidebar for site admin and site manager in another PR.

As revision CRUD panel users (site admin and site manager) will not change anything in revision records. This CRUD panel is ready-only.


It shows below changes of user selected institution:

  1. Institution ID (show for testing purpose temporary, will be removed soon)
  2. Initiative name
  3. Item type: Red Flag / Principle / Additional Criteria
  4. Item: The name of corresponding Red Flag / Principle / Additional Criteria
  5. Key: Which value has been changed. e.g. Red flag value / Principle rating / Principle is_na / Additional criteria rating / Additional criteria is_na
  6. Old value: The original value before change
  7. New value: The new value after change
  8. User: User name made this change
  9. Created: When did the change happen

TODO Items:

  1. To determine whether we should provide "Search" feature. If so, what fields will be searched for?
  2. Add a filter for initiatives, show revisions records of selected initiative only
  3. Show revisions records that belong to user selected institution only (not sure how to achieve this, can we use global scope?)

Screen shot:

image

dan-tang-ssd commented 1 year ago

I think it is now ready for review.

Regarding TODO Items:


To determine whether we should provide "Search" feature. If so, what fields will be searched for?

I did a google search for this topic, it seems that there is no easy way to hide the default "Search" feature.


Add a filter for initiatives, show revisions records of selected initiative only.

This is completed by finding revisions records for red flag, principle, additional criteria of the selected project and then combine an empty set and all 3 query results together by using orWhere()


Show revisions records that belong to user selected institution only (not sure how to achieve this, can we use global scope?)

This one is quite tricky...

In revisions table, there is no column to indicate which organisation it belongs to.

We can only find out the corresponding organisation very indirectly, i.e. find Red Flag / Principle / Additional Criteria model => Assessment model => Project model => Organisation ID

It seems "CRUD::addClause()" cannot be used here as it can only specify condition for database columns directly.

Possible workaround:

  1. Based on user selected organisation id, find out all related records of red flag / principle / additional criteria, only show revisions record with matched revisionable_type and revisionable_id.

It sounds complicated, and we may have performance issue when we have more and more records.

  1. Do not show any revision records at the beginning. User must select an initiative then only show related revision records in list view. As the filter only shows initiatives belong to user selected organisation, the revisions records to be showed must belong to user selected organisation.

This approach is simpler, and it is unlikely to have performance issue. But we may need to provide additioanl instructions on how to use this CRUD panel.


Screen shots:

Show empty set (no records) at the beginning image

Select an initiative in filter (it shows initiatives belong to user selected institution only) image

It shows empty set + red flags revision + principle revision + additional criteria revision only image

image

dan-tang-ssd commented 1 year ago

Just found one minor issue... User selected initiative is kept in Revision CRUD panel filter after changing institution. So the CRUD panel shows revisions records of previously selected initiative instead of empty set at the beginning...

Is it possible to reset Revision CRUD panel filter when user change institution?

User is viewing revisions record of institution A, then change institution image

User changed to institution B image

User enter Revision CRUD panel, it shows revisions records of previously selected initiative of institution A... User clicks "Remove filter" to clear the selected initiative in filter image

Revisions CRUD panel back to normal, showing empty set at the beginning image

dave-mills commented 1 year ago

Is it possible to reset Revision CRUD panel filter when user change institution?

I suggest the CRUD::disablePersistantTable() feature, that will not save the filter state between page loads. (I think this is fine, given how few people will be using this CRUD panel compared to the main user-facing pages).

We can only find out the corresponding organisation very indirectly, i.e. find Red Flag / Principle / Additional Criteria model => Assessment model => Project model => Organisation ID

The solution you found is pretty innovative and works well, although as you say it has the issue where it shows 0 entries until you add a project filter.

A "Laravel-y" solution to this is to use whereHas on the query. (Documentation here. This way, we can find all the entries that have a "revisionable" related item that fulfils a given subquery. We can nest whereHas indefinitely to work our way down the relationship chain. For example, we can add a global scope to the Revision model to only ever return entries for the selected organisation:

static::addGlobalScope('organisation', function(Builder $query) {
    $query->whereHas('revisionable', function(Builder $query) {
        $query->whereHas('assessment', function(Builder $query) {
            $query->whereHas('project', function(Builder $query) {
                $query->where('projects.organisation_id', Session::get('selectedOrganisationId'));
            });
        });
    });
});

The whereHas generates the appropriate "where exists()" subqueries in SQL to add onto the main query, so it's quite efficient from a database perspective.

This assumes that all 'revisionable' data models are related to the Assessment model, which is true for us right now. In the future we could extend this with more orWhereHas clauses to cover other options.

I've made a new commit that includes this global scope added to the Revision model, and also I refactored the initiative filter on the RevisionCrudController to make use of the revisionable relationship in the same way.

This is working for me, and has the added bonus of being able to show all revisions for the current organisation straight away. Let me know if it's working on your local test setup too?

dan-tang-ssd commented 1 year ago

Thank you Dave for your refactoring. It works really great now.

I have added a filter for item type. This should help user to find corresponding revisions records easier.

Um, do you think it is good to merge?

Screen shot: image

dave-mills commented 1 year ago

I have added a filter for item type. This should help user to find corresponding revisions records easier.

Nice - I agree this is probably the most useful filter to add.

I think this is good to merge :)

dan-tang-ssd commented 1 year ago

Deployment in staging env is completed. Performed testing in staging env, it works well.

image