search

specify / specify7

Specify 7
https://www.specifysoftware.org/products/specify-7/
GNU General Public License v2.0
62 stars 36 forks source link

Going to a record set triggers a form change #3259

Open realVinayak opened 1 year ago

realVinayak commented 1 year ago

Describe the bug If you click on 'Record Sets', then I am shown the list of record sets for that user. Try going from one record set to another and I get the dialog message 'This form has not been saved' even if I didn't change anything. A major issue because I wanted to know if I accidentally changed anything or not. This is in production by the way.

To Reproduce Steps to reproduce the behavior:

  1. Click on 'Record Sets' on the menu
  2. Click on any record set
  3. Click on 'Record Sets' again and go to another record set
  4. The dialog 'This form has not been saved.' shows up.

Expected behavior Shouldn't trigger the form change. Or at least tell me what changed.

image

realVinayak commented 1 year ago

The url in the screenshot: https://osuorton3823-edge.test.specifysystems.org/

realVinayak commented 1 year ago

Happens in Chrome, Edge and Firefox

maxpatiiuk commented 1 year ago

You have an unsaved form behind the dialog. Opening the dialog is fine and doesn't trigger the unload protect dialog. Actually clicking on a record set would replace the form with a record set, thus unload protect dialog is triggered to warn you about this. Not a bug. Though, if you think this is a UX issue, do you have suggestions on improving this?

realVinayak commented 1 year ago

Not showing the save dialog if there actually weren't changes! I discovered this while on production database and thought I changed the form

On Tue, Mar 28, 2023, 20:26 Max Patiiuk @.***> wrote:

Closed #3259 https://github.com/specify/specify7/issues/3259 as not planned.

— Reply to this email directly, view it on GitHub https://github.com/specify/specify7/issues/3259#event-8871037272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOSKLYXO3CKSZJUBRL5JBP3W6OFVVANCNFSM6AAAAAAWKRRVMU . You are receiving this because you authored the thread.Message ID: @.***>

maxpatiiuk commented 1 year ago

Not showing the save dialog if there actually weren't changes

Ah, that's a separate issue, not connected to record sets.

Two bugs here:

  1. When collection object is created, collection relationship is set, which triggers save blocker
  2. catalogedDatePrecision is set to 1 by default, which triggers save blocker
maxpatiiuk commented 1 year ago

TODO:

  1. Check if the two bugs mentioned in the previous commend (https://github.com/specify/specify7/issues/3259#issuecomment-1487878114) are still reproducible on xml-editor
  2. Fix them
maxpatiiuk commented 1 year ago

From @bronwyncombs:

Describe the bug Using the Browse in Forms button in query dialog through stats page requires save before moving pages.

To Reproduce Steps to reproduce the behavior:

  1. Go to statistics page
  2. Click on a statistic with query attached
  3. Click on Browse in Forms
  4. Use paginators
  5. See error

Expected behavior Should allow to click through forms since no edits are made.

Screenshots Desktop:

  • OS: macOS
  • Browser: Chrome
  • Specify 7 Version: 7.9-dev

Database Name: If applicable herb_rbge_3_31_23

Reported By Name of your institution

Additional context Database name or any other context about the problem here.

https://github.com/specify/specify7/assets/135047322/9b8b0b13-4cc0-4002-8eda-6eb97e8dfbb2

bronwyncombs commented 1 year ago

Browse in Forms doesn't have this issue in the main query builder, only through the stats query dialog. Do you still want these merged?

CarolineDenis commented 1 year ago

@maxpatiiuk @melton-jason , we met with @bronwyncombs for this issue because I couldn't reproduce. We discovered that it happens only if different tabs in the browser of the stat page are opened. If we have only one time Sp7 opened there is no issue. What are your thoughts?

maxpatiiuk commented 1 year ago

set a breakpoint on this line:

https://github.com/specify/specify7/blob/39cb18c84e94bd783fc10757b593039e683df8b8/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.js#L121

and when it hits, go up the stack to see who triggered the field change and why

there could be several different causes for this issue - would be best to fix them all

CarolineDenis commented 1 year ago

I cannot reproduce neither on my localhost or on the test panel. I know I saw it happening on Bronwyn's computer..

grantfitzsimmons commented 1 year ago

Browse in Forms doesn't have this issue in the main query builder, only through the stats query dialog. Do you still want these merged?

It looks like this is not the case for me:

https://herbrbge31323-v79-dev.test.specifysystems.org/specify/query/271/

https://github.com/specify/specify7/assets/37256050/57cb3c1d-58be-4e3c-a1c2-5cf7d62525be

This is likely an unrelated issue that @maxpatiiuk mentioned:

TODO:

  1. Check if the two bugs mentioned in the previous commend (https://github.com/specify/specify7/issues/3259#issuecomment-1487878114) are still reproducible on xml-editor
  2. Fix them

This has been a problem for a long time (at least since Specify 7.7+) where something is set upon the form loading that makes Specify think some edit has been made when it really has not.

bronwyncombs commented 1 year ago

Still works for me from query builder:

https://github.com/specify/specify7/assets/135047322/22b2dc22-848c-47eb-8421-753733493786

Only some have this issue from stats:

https://github.com/specify/specify7/assets/135047322/8487ce79-c032-4d8e-99c1-232e89673483

This is a new instance on a different database with one tab open at a time

CarolineDenis commented 1 year ago

Can we reproduce it on xml?

grantfitzsimmons commented 1 year ago

@bronwyncombs @CarolineDenis

Still works for me from query builder:

Screen.Recording.2023-07-10.at.11.47.54.AM.mov

Only some have this issue from stats:

screen-recording-2023-07-10-at-113003-am_LLKnihLE.mp4

This is a new instance on a different database with one tab open at a time

The issue is that you are testing different queries and different forms. These need to be the same to confirm whether behavior is consistent between them.

On the Preparation form, nothing is being set upon load, which doesn't prompt Specify to ask if you want to save.

On the Determination form, the date precision (presumably) is being set upon load, which prompts Specify to ask you if you want to save before you can navigate away.

This is the actual underlying issue^^^ I'd guess that the issues you're seeing with queries in stats will behave similarly in the query builder, and vice versa.

CarolineDenis commented 1 year ago

Could re-create when: stats > Paratype > browse in forms @maxpatiiuk where do we set resource.needSaved to true?

grantfitzsimmons commented 1 year ago

If this is not introduced in v7.9, we should move this to v7.9.1.

maxpatiiuk commented 1 year ago

Note, that this bug could be specific to a single record and a specific form. Note, that this bug could have many causes, but all result in a same symptom. Note, saving the record may cause the bug to disappear for this record (i.e let's say lat long type had an invalid value - sp7 front-end fixes it, which trigers a unload blocker - but if you save the record, next time you open it it is correct, so this "bug" does not happen again. Invalid lat long being fixed is a valid behavior, but there could be invalid behavior too - infinite loops where data is converted from one format to another and back).

maxpatiiuk commented 1 year ago

@CarolineDenis this line I sent is the only place where we set resource.needSaved=true:

https://github.com/specify/specify7/blob/39cb18c84e94bd783fc10757b593039e683df8b8/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.js#L121

But, this function in turn is called in several places (in 3 places I belive, one of which is .set())

To debug this issue:

  1. Find a record and a form on which you can consistently reproduce this issue (that could be either on the test panel or on localhost)
  2. Set a breakpoint on the line I sent above
  3. When breakpoint hits, double check that this.specifyModel.name (or this.specifyTable.name if on xml-editor) matches the table name you expect (i.e CollectionObject rather than PickList or TreeDefItem - you might see the later as those are created and edited by front-end behind the scenes, especially on initial page load)
  4. Then, simply follow the stack track to see who called the .set() function and why and with what value and what was the previous value. See https://developer.chrome.com/docs/devtools/javascript/reference/#call-stack
CarolineDenis commented 1 year ago

"3.When breakpoint hits, double check that this.specifyModel.name (or this.specifyTable.name if on xml-editor) matches the table name you expect (i.e CollectionObject rather than PickList or TreeDefItem - you might see the later as those are created and edited by front-end behind the scenes, especially on initial page load)" The name is correct

CarolineDenis commented 1 year ago

@melton-jason can this be closed?

melton-jason commented 1 year ago

I am unsure whether this should be closed or not, as the underlying cause of the issue is very far-reaching and pops up often.

However, this issue has become synonymous with the problem that has been fixed as of #3774, which is why I closed it. Each instance of this problem can be diagnosed via adding a breakpoint to https://github.com/specify/specify7/blob/39cb18c84e94bd783fc10757b593039e683df8b8/specifyweb/frontend/js_src/lib/components/DataModel/resourceApi.js#L121 However, exactly what caused that line to be run will probably vary for each iteration of the issue, and that may warrant opening separate issues.

This problem as a whole however can probably not be considered entirely 'fixed' until we rewrite the ORM and get away from Backbone.

bronwyncombs commented 1 year ago

Encountered this today on sdnhmpaleo_3_31_23 while using Browse in Forms for Locality records.

https://github.com/specify/specify7/assets/135047322/c9702a48-cc77-4196-b023-6811b25f6dfd

grantfitzsimmons commented 1 year ago

Solved by #3959

grantfitzsimmons commented 1 year ago

https://dbfordocskufish-v79-dev.test.specifysystems.org/specify/overlay/merge/Agent/?records=1010%2C1028%2C1069%2C1102%2C1514

https://github.com/specify/specify7/assets/37256050/f41816ba-e76b-4810-972b-67c2a1a9b3a8

Going through Determination records causes this issue for me in agent merging

carlosmbe commented 8 months ago

Reopened. Still present in Locality Record Sets. Happens in both https://github.com/specify/specify7/pull/3959 and https://github.com/specify/specify7/pull/3774

carlosmbe commented 8 months ago

https://github.com/specify/specify7/assets/53784701/b2e98580-68e3-4db5-87ba-9b923c675c70

lexiclevenger commented 4 months ago

This also happens when navigating through a record set created using the WorkBench.

https://github.com/specify/specify7/assets/164079735/2d799c46-2647-49f2-a054-d1e8477d2242

emenslin commented 2 months ago

Can recreate in edge (7.9.6) but only with one RS. There are probably other RS in other dbs that can also recreate this issue but this is what I found. Not sure why this one is different though, maybe just because it's an old WB upload?

https://github.com/user-attachments/assets/4fc9227b-ab3f-4149-987e-2ec4512c4c7b

emenslin commented 2 months ago

From @bronwyncombs

Describe the bug A clear and concise description of what the bug is.

the 8th record in this record set specifically triggers unload protect: demofish record set

Happens on this record set in fwri when you skip to the last record and go backward: fwri record set

  • OS: macOS
  • Browser: Chrome
  • Specify 7 Version: v7.9
  • Database Name: fwri_1_9_24 and sp7demofish_2_9_24

Additional context

related issue: https://github.com/specify/specify7/issues/3259