Feat allow admin to edit observations #363

santostiago · 2022-11-13T21:45:31Z

Task
When a flag to override observations is on, the admins can change observations in ways that are forbidden under normal circumstances.
The env var OVERRIDE_OBSERVATION_STATUS should be set to TRUE.

Pull request checklist

Please check if your PR fulfills the following requirements:

Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
Check the commit's or even all commits' message styles matches our requested structure.
Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Admins cannot edit all the fields in observations

Issue Number: #181285745

What is the new behavior?

Admins can now edit all the fields in observations

Does this introduce a breaking change?

Yes
No

Other information

santostiago · 2022-11-13T21:47:45Z

@tsubik I updated the active admin addons so that we could use nested fields.
This removes the ability to have image/file rows/columns in the admin.
Maybe I could check the code of the old gem to allow displaying images/file types, but it's not super important so I didn't add it yet.

santostiago · 2022-11-13T21:50:19Z

Also, the field law is not dependent on the country/subcategory.
I could add some custom JS to make those ajax requests, but I thought it was too much trouble for the gains we'd get.

tsubik · 2022-11-14T10:37:02Z

app/admin/observation.rb

@@ -413,6 +413,7 @@ def self.observations
 end

 form do |f|
+ allow_override = ENV.fetch('OVERRIDE_OBSERVATION_STATUS', nil)&.upcase == 'TRUE'


why not just ENV.fetch('OVERRIDE_OBSERVATION_STATUS', 'false').upcase == 'TRUE' and why do we need to have it as a flag? Do you think we want to change it back and forth?

@tsubik yeah, this is supposed to be for them to temporarily edit observations that were created wrongly.
I'm guessing this will happen more times in the future.

tsubik · 2022-11-14T11:11:21Z

app/admin/observation.rb

+ if f.object.observation_type == 'operator'
+ f.input :fmu_id, as: :nested_select,
+ level_1: {
+ attribute: :country_id,


indent level is weird here.

@tsubik indeed... I'll check it. I think it was Rubocop that changed it.

Those nested selects do not work properly, the list is not filtered

@tsubik I didn't understand the screenshot. Which fields do not work? I tested it locally and it's filtering by the right field.

Let me check it once again, as it was long time ago and I don't remember what was not working. Yeah, the screenshot could be better as it's not showing much.

@santostiago both Operator and FMU selects do not work correctly

@tsubik ohh.. now I get what you mean. The filtered query is not being used correctly, no. Because we're using a translation table, it seems like ransack cannot interpret it properly.

For example, with the Democratic Republic of Congo selected, when I search for the operator Andre the ajax query made is:
http:https://localhost:3000/admin/producers?order=operator_translations.name_desc&q[groupings][0][m]=or&q[groupings][0][operator_translations.name_contains]=andre&q[combinator]=and&q[country_id_eq]=7

I'm not sure what these 2 params do:

q[groupings][0][m]=or

&q[combinator]=and

I need to check ransack. I'm also not sure if the right way to query the name is by using q[groupings][0][operator_translations.name_contains]=andre. But this is done by the gem. It might be easier if I just manage to change the controller of the Operator AA resource to handle this properly.

I think there is a task to remove name from translated fields for operator (maybe that would help here), not sure if FMU as well.

it would solve it, yes. (I just dug into ransack code, and the issue is that when there's a query with groupings, ransack searches for the field name in his context - and he doesn't have the translations in his context).

I made a quick hack to fix it. Not an ideal solution, but I wouldn't waste more time on this.

tsubik · 2022-11-14T11:24:26Z

app/models/observation.rb

+ # Returns whether the admin can override the status of an observations and its fields.
+ # This is defined by the ENV var `OVERRIDE_OBSERVATION_STATUS` which should be 'TRUE'
+ def admin_can_override?
+ user_type == :admin && ENV.fetch('OVERRIDE_OBSERVATION_STATUS', nil)&.upcase == 'TRUE'


What is user type here, and where is it set to be :admin?

Looks like for admin panel validation of status changes will never be invoked, right?

Looks like for admin panel validation of status changes will never be invoked, right?

Yup.

I meant, I have not seen setting user_type to admin anywhere that's why I'm asking if that's a mistake to check it here?

@tsubik you're absolutely right.
In fact, the rules seemed to have been wrong before. We were just validating the transitions when it came from the observations tool.
I'm changing it.

tsubik · 2022-11-14T11:30:01Z

app/models/observation.rb

+ return unless operator_id_was == operator_id
+
+ old_operator = Operator.find_by id: operator_id_was
+ ScoreOperatorObservation.recalculate! old_operator if old_operator.present?


Changing operator is still disabled, so how this code could be invoked? Or maybe you forgot to remove disabled from operator input. I thought operator should be changeable and that was the point of the feature?

@tsubik mmm... I'm not sure what you're referring to. The operator can be modified (what cannot be is the type of observation, because I didn't want to check all the consequences and I don't think that should change).

I cannot modify operator, it has disabled: operator in the input. Select stays disabled in admin panel.

santostiago · 2022-11-14T20:27:12Z

spec/models/observation_spec.rb

@@ -71,28 +71,80 @@


 describe 'Validations' do
- describe 'Status changes' do
+ describe '#status_changes' do
+ let(:country) { FactoryBot.create(:country) }


I'm pretty sure I didn't write FactoryBot.. Could it be that Rubocop did it? (or maybe I'm going crazy :P )

santostiago · 2022-11-21T09:15:48Z

@tsubik I modified it, but I'll wait for Sophie to know if this is what they want.
Previously, we were allowing admins to change the status of an observation to whatever they wanted. But I'm not sure if that should be the case.
Depending on what Sophie replies, I'll update the PR.

santostiago · 2022-11-22T20:11:54Z

Ok, so we decided to just go with having admins and bo_admins

tsubik · 2022-11-22T20:27:58Z

Alright, so many modifications are needed in that case :/

santostiago · 2022-11-26T21:28:01Z

@tsubik ok, so:

Admins can modify everything in the observations
BO Managers can modify the status and admin comments
Batch actions are hidden unless there's a flag

The only thing that was not very clear on Sophie's spec was if the admins could modify the status at will. I'm going to confirm that.

tsubik · 2022-11-28T13:18:32Z

app/admin/observation.rb

- resource.update(validation_status: Observation.validation_statuses['QC in progress'])
- redirect_to collection_path, notice: 'QC in progress for observations'
+ resource.validation_status = Observation.validation_statuses['QC in progress']
+ notice = resource.save ? 'Observation approved' : 'Observation not modified'


Why all messages are changed to "observation approved"?

Mmmm.. right. I'll change them to the previous one.

tsubik · 2022-11-28T13:19:26Z

app/admin/observation.rb

- redirect_to collection_path, notice: 'QC started'
- end
+ # Bulk actions should be available only if this env flag is set to `true`
+ if ENV.fetch('BULK_EDIT_OBSERVATIONS', 'false').upcase == 'TRUE'


If we don't need that, I would vote to just remove it and not take space here ;)

Sophie said it shouldn't be done. But I'd bet it's gonna be needed again in a year or so.
Do you think we should just get rid of it?

Then, maybe better to keep them without disabling?

But keeping code we should also maintain it, and we should not maintain something that might be needed in future, but probably don't, so either we remove it or keep it. That's my opinion.

As discussed with Sophie, let's keep this hidden under an ENV var.

tsubik · 2022-11-28T15:01:16Z

app/admin/observation.rb

- operator = object.operator_id.present? ? true : false
- fmu = object.fmu_id.present? ? true : false
- government = object.government_ids.present? ? true : false
+ allow_override = current_user.user_permission.user_role == 'admin'


The form got very big and confusion, but maybe that's ok. Just thinking if for bo manager we can only show necessary fields if there are any and the fields that the user can change. Not sure if that is a good idea

@tsubik I thought of it, but they need to see those in order to approve or not and to comment.

Ok. Good point

santostiago · 2022-11-30T19:15:56Z

Going to rebase it now.

santostiago · 2022-11-30T21:02:24Z

app/admin/observation.rb

- redirect_to collection_path, notice: 'QC started'
- end
+ # Bulk actions should be available only if this env flag is set to `true`
+ if ENV.fetch('BULK_EDIT_OBSERVATIONS', 'TRUE').upcase == 'TRUE'


@tsubik I ended up changing it to be on by default and we add the var with false in the server.
Doing it the other way was proving to be complicated to test (I was trying to remove the constant Admin::ObservationsController and then reloading the file, but I'd have to reload it using the AA loader and it wasn't working)

tsubik · 2022-12-02T11:46:58Z

BO Managers can modify the status and admin comments

Hey @santostiago, but I see there are no valid status transitions for bo_manager.

santostiago · 2023-01-14T14:03:17Z

@tsubik we're sending the requests from the observations admin page as admin, so it will have the same permissions (this can be improved, because we should instead check who made the request and set the permissions there, but I'll do it in another PR).
As for the filters, they seem to be working: I just filtered by one country, and the only operators shown are for that country. There's an error though which I'm going to check now: we're sending only the first page (20 results). I'll check why now.

Allows admins to edit most of the fields of observations, and makes bo_managers only capable of modifying the 'status' and 'admin comment' fields.

santostiago · 2023-01-15T20:36:54Z

@tsubik can you review? The solution is a bit hacky, but I didn't want to waste more time with this.

tsubik · 2023-01-16T14:04:50Z

app/admin/operator.rb

@@ -280,6 +282,24 @@
 end

 controller do
+ def index


Problem is not only for operator but for many other attributes that are using this component, category, subcategory and others. Let me think about it.

With hotwire that would be easy to just refresh form on every country select change or other select changes.

use different ransack hack to work with globalize attributes

santostiago added the Code Review Requested label Nov 13, 2022

santostiago requested a review from tsubik November 13, 2022 21:45

santostiago self-assigned this Nov 13, 2022

tsubik reviewed Nov 14, 2022

View reviewed changes

santostiago commented Nov 14, 2022

View reviewed changes

santostiago force-pushed the feat-allow_admin_to_edit_observations branch from a592118 to 9c477f8 Compare November 26, 2022 21:25

tsubik reviewed Nov 28, 2022

View reviewed changes

santostiago force-pushed the feat-allow_admin_to_edit_observations branch from 9c477f8 to 6d72ac3 Compare November 30, 2022 19:15

santostiago force-pushed the feat-allow_admin_to_edit_observations branch 2 times, most recently from e05e192 to 66730ac Compare November 30, 2022 20:54

santostiago commented Nov 30, 2022

View reviewed changes

santostiago requested a review from tsubik November 30, 2022 21:02

santostiago force-pushed the feat-allow_admin_to_edit_observations branch 2 times, most recently from 764dbec to fe3164a Compare January 8, 2023 19:06

santostiago added 2 commits January 15, 2023 21:35

feat(Observations): Allow admin to override observations

fd5bf89

Allows admins to edit most of the fields of observations, and makes bo_managers only capable of modifying the 'status' and 'admin comment' fields.

gemfile: Update active admin addons

c77f507

santostiago force-pushed the feat-allow_admin_to_edit_observations branch from 93f0771 to c77f507 Compare January 15, 2023 20:36

tsubik reviewed Jan 16, 2023

View reviewed changes

tsubik and others added 2 commits January 16, 2023 17:12

use different ransack hack to work with globalize attributes

5e5bf5c

Merge pull request #393 from wri/ransack_changes_for_obs_form

02b64de

use different ransack hack to work with globalize attributes

santostiago requested a review from tsubik January 16, 2023 21:04

forgot to remove hack for fmu

4f7838e

tsubik approved these changes Jan 17, 2023

View reviewed changes

tsubik merged commit b842bd7 into develop Jan 17, 2023

tsubik deleted the feat-allow_admin_to_edit_observations branch January 17, 2023 10:28

Feat allow admin to edit observations #363

Feat allow admin to edit observations #363

Conversation

santostiago commented Nov 13, 2022

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

santostiago commented Nov 13, 2022

santostiago commented Nov 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santostiago Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santostiago commented Nov 21, 2022

santostiago commented Nov 22, 2022

tsubik commented Nov 22, 2022

santostiago commented Nov 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santostiago commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsubik commented Dec 2, 2022

santostiago commented Jan 14, 2023

santostiago commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santostiago Jan 14, 2023 •

edited

Loading