search

Rothamsted-Ecoinformatics / farm_rothamsted

Custom farmOS features for Rothamsted Research.
GNU General Public License v2.0
6 stars 1 forks source link

Deploy 2.21 #660

Closed paul121 closed 3 months ago

paul121 commented 4 months ago

Okay, pending a review and :+1: from @aislinnpearson , I think we're ready to deploy this. I still need to merge my PR and tag a release, but just jotting some notes down while it is fresh:

mstenta commented 4 months ago

Great! Ready whenever you are @aislinnpearson. I have the farmOS 3.2.2 update ready to go out as well, so I will deploy them together.

aislinnpearson commented 4 months ago

Hi @paul121

Thanks so much for this. I've been through and done the testing now. It all works as I think it should, but I did find a few complexities (see below) and re-wrote some of the descriptions which I hope will add clarity (but if you can improve on m wording again, please go for it).

Comments: I added comments to all log types, asset types and research entities. They all work.

Alerts: I tested these both on and off for all researchers, logs and research programs. They all work. I didn't test 'all emails alerts' because I assume that functionality hasn't changed?

The issues I came across were:

1. Commenting on logs: Error in the e-mail alert logic? There was some strange behaviour with the e-mail alerts when I commented on an log. I think what was happening is that If you comment and flag you get two e-mails: an edited log (because of the flag) and, for some reason, a new log notification. However, if you only comment, there is no notification?

2. Error messages: understanding the logic

image

I really like the above error message - great catch/ value add. I hadn't thought about this. I just wasn't sure about the logic, which I'd also like to document. From my tests I think I get the above error if:

3. Error messages: what text should be displayed.

I was wondering if we can make the error message text a bit clearer. Perhaps something like 'Warning: [Researcher Name] cannot be alerted about the [changes you have made]/ [Log you have created] because they do not have a FarmOS user account associated it their Researcher Profile. Please contact your FarmOS administrator if this is a problem."

4: Couldn't create plots for experiments I am associated with This one is incidental, but I realised I couldn't create plots for experiments I am named on even though I had set myself up as an admin. I get the below error message. I can however create plots for experiments where I am not named. See for example: https://pr-14-l2fuguy-2mu45anerqtvq.uk-1.platformsh.site/plan/1/plots/create (where I was unsuccessful) and https://pr-14-l2fuguy-2mu45anerqtvq.uk-1.platformsh.site/plan/3/plots (where I was successful).

image

5: Making the descriptions more user friendly:

The below isn't perfect, please feel free to change it if you can see any obvious improvements

Email notifications: Switch on/ off all e-mail notifications. Please note that there are some e-mail notifications which cannot be switched off for authentic purposes. For example if someone creates a Research Profile on your behalf, or if someone names you on a Research Program, Proposal, Experiment or Design.

Researcher Profile Notifications: Switch on/off e-mail notifications relating to changes to your Researcher profile in FarmOS. If this is switched off, you will no longer receive notifications if someone other than you edits your Researcher profile (e.g. an administrator). This is on by default. If you leave it on, you will receive e-mails as soon as any changes are made.

Research Program Notifications: Switch on/off e-mail notifications relating to changes to any Research Programs you are associated with in FarmOS. If this is switched off, you will no longer receive notifications if someone other than you edits a Research Program where you are named as a PI (e.g. an administrator). This is on by default. If you leave it on, you will receive e-mails as soon as any changes are made.

Log Notifications: Switch on/off e-mail notifications for logs. If this is switched of you will no longer receive notifications when someone (e.g. farm staff) adds or edits the logs associated with the experiments you are named on. This is on by default. If you leave it on, you will receive e-mails as soon as new logs are added or any changes are made.

6: 'Flag this log as needs review' should be on by default if log is already flagged

I noticed that if a log is already flagged, the toggle still defaults to 'off' and was wondering if we could change this so it is on if a log is already flagged as well as use the toggle to remove the flag? We might then also want to change the description, although I am not sure what to. Maybe something to chat about tomorrow?

aislinnpearson commented 4 months ago

Notes from the call on 31/05/2024

paul121 commented 4 months ago

Okay, I think I fixed the issues re: sending emails for Logs + Log comments. There were a couple small gotchas. I'm not sure I was able to reproduce the exact same error message we saw (seemingly related to a user/researcher without an email?) but added a check to prevent this. Like we mentioned, most users in production will have emails so this might not be a common issue.

Re: experiment plots, I don't think there are actually any issues...

I can however create plots for experiments where I am not named. See for example: https://pr-14-l2fuguy-2mu45anerqtvq.uk-1.platformsh.site/plan/1/plots/create (where I was unsuccessful) and https://pr-14-l2fuguy-2mu45anerqtvq.uk-1.platformsh.site/plan/3/plots (where I was successful).

Aha, you are actually named as a PI for the "Test" Research Program, which does give you access to create plots for that experiment plan: https://pr-14-l2fuguy-2mu45anerqtvq.uk-1.platformsh.site/rothamsted/program/2

Also, the unsuccessful plot creation is a small edge-case bug. The Experiment plan did not have a "Study Period ID" set. This was previously not required but we now require it and use it in the plot name. This shouldn't be an issue in production because your old plans either already have plots, or new plans will require a study period ID when they are created.

paul121 commented 4 months ago

Okay, made the following changes, will work on tagging a release...

5: Making the descriptions more user friendly: 6: 'Flag this log as needs review' should be on by default if log is already flagged

paul121 commented 4 months ago

Okay! 2.21.0 is tagged and ready to deploy @mstenta

mstenta commented 3 months ago

Great! I will plan to deploy it this afternoon (this evening UK time).

aislinnpearson commented 3 months ago

Thanks both!!

mstenta commented 3 months ago

Encountered the following error during the update:

  [Error]                                                                      
  Call to undefined function Drupal\farm_rothamsted_experiment_research\Entit  
  y\farm_comment_base_field_definition()

Postponing until tomorrow so we can figure out what's causing it.

paul121 commented 3 months ago

We think we can avoid this error if we enable the farm_comment module before running updates. Lets try deploying the new release 2.21.1

mstenta commented 3 months ago

That did the trick! All instances are update and have the new farm_rothamsted_comment module installed!

aislinnpearson commented 3 months ago

Deployed 04 June 2024