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

Add lastUpdated property to Form #1479

Merged
merged 1 commit into from
Feb 21, 2023
Merged

Add lastUpdated property to Form #1479

merged 1 commit into from
Feb 21, 2023

Conversation

Chartman123
Copy link
Collaborator

This fixes #1462

Signed-off-by: Christian Hartmann [email protected]

@Chartman123 Chartman123 added enhancement New feature or request 2. developing Work in progress labels Feb 2, 2023
@Chartman123 Chartman123 added this to the 3.1 milestone Feb 2, 2023
@Chartman123
Copy link
Collaborator Author

@jotoeri could you please point me in the right direction, why my setLastUpdatedTimestamp function in the forms service doesn't work?

@jotoeri
Copy link
Member

jotoeri commented Feb 2, 2023

You need to update the Form Entity to the Db with $this->formMapper->update($form);. Currently you only retrieve the Form-Entity, set the Timestamp on it, but don't write it back to Db.
And you first need to store the form entity you get from findById. 😉

@Chartman123
Copy link
Collaborator Author

And you first need to store the form entity you get from findById. wink

🤦🏻

$this->formMapper->update($form);

Thanks, this was the missing part.

@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch 5 times, most recently from b1b4486 to 95b339f Compare February 2, 2023 21:26
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #1479 (4862112) into main (0eefbbe) will decrease coverage by 0.10%.
The diff coverage is 20.45%.

❗ Current head 4862112 differs from pull request most recent head d179439. Consider uploading reports for the commit d179439 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1479      +/-   ##
============================================
- Coverage     39.56%   39.47%   -0.10%     
- Complexity      556      560       +4     
============================================
  Files            53       54       +1     
  Lines          2320     2333      +13     
============================================
+ Hits            918      921       +3     
- Misses         1402     1412      +10     

@Chartman123
Copy link
Collaborator Author

@jotoeri The backend stuff should be ready now. However, I'm not sure how to proceed with the frontend, so that a changed form for example will also result in a reordered forms list in the AppNavigation. Shall we split this into a second PR?

@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 8a28129 to 5ad91b5 Compare February 2, 2023 23:13
@Chartman123 Chartman123 added php PHP related ticket 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 2, 2023
@Chartman123 Chartman123 marked this pull request as ready for review February 2, 2023 23:14
@jotoeri
Copy link
Member

jotoeri commented Feb 3, 2023

Hmm, to avoid confusion, i think we should shift it up, once updated/edited. That will of course not cover submissions, but will make clear the principle.

  • Should suffice to just use the timestamp on the Frontend within a session, no need to have the exact timestamp of the backend.
  • To not pass the event everywhere through our components you can also use the nextcloud/event-bus package.

And i wouldn't do it separately. Its closely connected.

@Chartman123 Chartman123 added javascript Javascript related ticket design Related to the design labels Feb 4, 2023
@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from e7b4b42 to d0e2c39 Compare February 4, 2023 15:24
@Chartman123
Copy link
Collaborator Author

@jotoeri It's already working for updated form properties. However, I still get some errors when updating question properties and question options. Perhaps you could already have a look at it... I will only be able to get back to work tomorrow in the evening.

@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 9327503 to f3df198 Compare February 5, 2023 02:37
@susnux
Copy link
Collaborator

susnux commented Feb 5, 2023

I still get some errors when updating question properties and question options.

Do you still get those errors? For me it works perfectly when changing question properties like required or shuffle options.

@Chartman123
Copy link
Collaborator Author

@susnux I figured out the question related problems... now just the options/answers are missing.

I'm a little clueless what is triggered by the $emit('update:options',...) in dropdown and multiple questions. There is no @update:options in any of the vue components...

@susnux
Copy link
Collaborator

susnux commented Feb 5, 2023

I'm a little clueless what is triggered by the $emit('update:options',...) in dropdown and multiple questions. There is no @update:options in any of the vue components...

I think it is used in views/Create.vue: v-bind.sync="form.questions[index]". As this should bind :options.sync.

@Chartman123
Copy link
Collaborator Author

@susnux @jotoeri I've now added the re-sorting for changed options. My last problem is that this also moves the form up just on clicking inside the "add new option" input box. At this time, the options haven't change in the database, so basically the sorting is "wrong".

@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 10045cc to a896802 Compare February 5, 2023 20:34
@Chartman123
Copy link
Collaborator Author

@susnux Your new ApiControllerTest is killing me... 😆 Could you please help me fix the failing checks here?

@susnux susnux force-pushed the feat/sort-recent-forms branch from 495140c to c2ef873 Compare February 6, 2023 12:42
@susnux
Copy link
Collaborator

susnux commented Feb 6, 2023

You need to mock the time() function as the controller sets the lastUpdated value based on it. (see my commit)

@Chartman123
Copy link
Collaborator Author

Thank you! Now I can polish the integration tests and then everything should be fine :)

@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 4862112 to 965f007 Compare February 6, 2023 17:00
src/Forms.vue Outdated Show resolved Hide resolved
@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 965f007 to 15ff608 Compare February 11, 2023 20:46
@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 15ff608 to 65e9fcd Compare February 18, 2023 20:51
@jotoeri jotoeri modified the milestones: 3.1, 3.2 Feb 20, 2023
Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So - now got into this one. Two small comments, maybe also squash a bit, but apart from that, it should be fine by me.

lib/Migration/Version030100Date20230202175747.php Outdated Show resolved Hide resolved
lib/Migration/Version030100Date20230202175747.php Outdated Show resolved Hide resolved
Signed-off-by: Christian Hartmann <[email protected]>
@Chartman123 Chartman123 force-pushed the feat/sort-recent-forms branch from 65e9fcd to d179439 Compare February 20, 2023 23:27
@Chartman123
Copy link
Collaborator Author

So - now got into this one. Two small comments, maybe also squash a bit, but apart from that, it should be fine by me.

Fixed them and squashed the commits. I think it's ready to go now :)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, thank you @Chartman123! :) (Can't review the code, so only leaving comment instead of approval.)

@Chartman123 Chartman123 requested a review from susnux February 21, 2023 10:13
Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Chartman123 Chartman123 merged commit 3181423 into main Feb 21, 2023
@Chartman123 Chartman123 deleted the feat/sort-recent-forms branch February 21, 2023 14:06
@susnux
Copy link
Collaborator

susnux commented Feb 21, 2023

@Chartman123
I tested this using mariaDB, so I did not noticed it, but this breaks updating a server that runs on SQLite.
The comment option on the new database column does not work with SQLite:
https://www.doctrine-project.org/projects/doctrine-dbal/en/current/reference/schema-representation.html#common-options

In ExceptionConverter.php line 82:
                                                                                                     
  An exception occurred while executing a query: SQLSTATE[HY000]: General error: 1 incomplete input  
                                                                                                     

In Exception.php line 30:
                                                      
  SQLSTATE[HY000]: General error: 1 incomplete input  
                                                      

In Connection.php line 72:
                                                      
  SQLSTATE[HY000]: General error: 1 incomplete input  
                                                      

I do not know why a clean install work (no forms installed before), it only fails if a partial migration is required.

@Chartman123
Copy link
Collaborator Author

Strange, I copied the code from some older migration... 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Related to the design enhancement New feature or request javascript Javascript related ticket php PHP related ticket
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation: Sort forms by recently edited / responses received
4 participants