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.

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

FormSynopticRA1 Saving of Records #38

Open
Patowhiz opened this issue Mar 22, 2018 · 16 comments
Open

FormSynopticRA1 Saving of Records #38

Patowhiz opened this issue Mar 22, 2018 · 16 comments

Comments

@Patowhiz
Copy link

Patowhiz commented Mar 22, 2018

@dannyparsons @smachua @mhabimana .The fields naming in form_synoptic_2_ra1 datatable doesn't match the element id in the obselement datatable. The old forms are saving records of the observation elements to the wrong columns. For instance Geopotential Height-hh which is an element with Id 185 is being saved to column val_Elem196., yet an element with Id 196 is Cloud Amnt  High Lvl . Is this done intentionally?

shadrackkibet pushed a commit to shadrackkibet/Climsoft that referenced this issue Mar 28, 2018
maxwellfundi pushed a commit that referenced this issue Jun 8, 2018
@smachua
Copy link

smachua commented Jun 13, 2018

Table form_synoptic_2_ra1 had been modified to match with the code. Actually a number of tables had been modified. The SQL scripts that were used for those modifications had already been sent to Maxwell. I have attached them here. Please run them to have the database modified.
modify_tables.txt

@Patowhiz
Copy link
Author

@smachua @mhabimana below is an image of form_synoptic_2_ra1 . It shows how currently we are associating each control with it's corresponding element code. With the new database there has been changes to the field in the table and therefore we also need to make changes to this form. I've gone through the elements codes in obselement table but I'm still not quite sure with some of the descriptions. In particular, help me with setting the correct element code for the corresponding controls. I presume the easiest way would be to state which controls have the wrong element codes and their corresponding correction.

frmsynoptic

@Patowhiz
Copy link
Author

@smachua @mhabimana I've also noticed that some of the element names do not completely correspond to the label we have used in this form. For instance 107 is labelled Pressure Reduced to MSL-P yet its element name is Pressure Sea Level. Is that done intentionally or these are their formal alternative names?

@mhabimana
Copy link

@Patowhiz :107 is labelled Pressure Reduced to MSL-P or Pressure Sea Level. It is the same.

@Patowhiz
Copy link
Author

Patowhiz commented Jul 13, 2018

@mhabimana @smachua @dannyparsons @isedwards @maxwellfundi below is a screenshot of form_synoptic_ra1 form demonstrating the corresponding element code.s for all the elements that on the form. Flag169, Flag170 and Flag171 do not exist in the database and entity framework. Their corresponding Val_Elem169,Val_Elem170 and Val_Elem171 do exist. For this form to be usable, the database and the EF will have to be fixed. I believe they are supposed to store the values of element 169,170 and 171. For now an error message will popup to show this. I welcome any suggestion on this.

frmsynopticnew

@mhabimana
Copy link

mhabimana commented Jul 13, 2018

@Patowhiz @dannyparsons @isedwards @maxwellfundi @smachua : I had a discussion with Samuel yesterday and he is suggesting that since you are using the test database which is not up to date,he is going to update both the test database and the empty database with the latest changes and share the script with the team to avoid any change of the model which may be the result of conflicts with the existing work on climsoft.

@smachua
Copy link

smachua commented Jul 15, 2018

@Patowhiz @dannyparsons @isedwards @maxwellfundi: Following Marcellin's comment I have attached a file containing SQL scripts for all the modifications that have been done on different tables. The modifications became necessary while implementing AWS and TDCF functions. form_synoptic_ra1 has been modified as well.
tables_modify.txt

@smachua
Copy link

smachua commented Jul 15, 2018

Me and Marcellin are in the process of merging our code and that from AMI team. We therefore suggest that any additions should be done after the merging has succeeded. From the AMI code it seems database connection is hardwired in the code and pointing to 'mariadb_climsoft_test_db_v4 or we're missing something. We have always recommended that the code should allow data connection at run time. Any comment on this is highly welcome.

@mhabimana
Copy link

@africanmathsinitiative/climsoft : Just to add that the above script should be renamed as tables_modify.sql and then executed on both mariadb_climsoft_db_v4 and mariadb_climsoft_test_db_v4 to update both databases with the latest modifications.

@rdstern
Copy link

rdstern commented Jul 16, 2018

Great to hear that you are in the process of updating. We have discussed with the AMI team and they are not currently working on the code until you finish.

Samuel queries the fact that the AMI code currently includes hard-wiring in the code to the 'mariandb-climsoft_test_db_v4'. My understanding is that the hard-wiring was in the code as they received it. They have concentrated on the data entry forms, so have not changed the code in other areas. Hence in areas other than data entry the updated code is likely to be in Samuel's version. The reason this may appearing updated in the AMI version is that given the database was hard-wired they needed to change the name to the test one they were using.

@isedwards
Copy link
Member

It's important that we don't just put the two versions of the code together manually (e.g. by copying and pasting). The recent developments by AMI have a long history of commits discussing and explaining all the changes that have been made (these would al be lost if the final source is just copied).

There are other advantages to keeping the git history. This will all happen automatically when using git to merge the versions and just resolving the files that it identifies need attention.

@isedwards
Copy link
Member

@rdstern - Samuel is correct - the new code currently contains the connection string with details of the development database and test database because Entity Framework was using this during development. This can now be replaced by passing the required connection string to the DbContext constructor at run time.

@rdstern
Copy link

rdstern commented Jul 18, 2018

I can well appreciate the importance of merging so that the history is kept. We need to get the version for testing as soon as possible, but it is also our baseline and so important that it is done so that it can easily be advanced in the future.

@mhabimana are you and @smachua comfortable with this, or is it be something where help from @isedwards or @maxwellfundi or @dannyparsons or @volloholic would be useful?
,

@smachua
Copy link

smachua commented Jul 18, 2018

@rdstern and @isedwards. We (@smachua and @mhabimana) have no problem with merging through Github. We had successfully put all code (samuel, victor and AMI) together to test how the combined code works then upload it to Github. It worked ok only that the code by AMI had database connection hardwired to the test database and lacked database connection flexibility. As such the new forms developed by AMI couldn't be properly used. It was a big concern.
Now if what @isedwards has indicated that the connectivity issues can be resolved then the testing version will be ready soon after that. Therefore let AMI team work with speed and modify the code where necessary so that everything can be finalized.

@Steve-Palmer
Copy link

@Patowhiz @smachua and @mhabimana - thinking about the problem with the Entity Framework, we probably need to do some work on aligning this set with other work going on in WMO on entity descriptions. I will check with Jeremy Tandy, find out what the state is, and produce some ideas. There was work to produce standardised long and short descriptors for observation elements. However, probably not an immediate task.

@Patowhiz
Copy link
Author

@Steve-Palmer great idea, looking forward to such a proposal.

shadrackkibet pushed a commit to shadrackkibet/Climsoft that referenced this issue Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants