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

MIR-1343 Do not render mir-admindata-box staticly #1040

Merged

Conversation

Possommi
Copy link
Contributor

@Possommi Possommi commented Jul 31, 2024

MIR-1343 and MIR-1341

requires MyCoRe-Org/mycore#2215 to render the dates properly

@Possommi Possommi requested a review from sebhofmann July 31, 2024 08:51
@Possommi
Copy link
Contributor Author

Possommi commented Jul 31, 2024

@toKrause should MIR-1341 included in this PR?

@Possommi Possommi requested a review from toKrause July 31, 2024 08:57
@toKrause
Copy link
Contributor

toKrause commented Jul 31, 2024

@toKrause should MIR-1341 included in this PR?

Would be okay for me and make things easier, organizationally.

@toKrause
Copy link
Contributor

toKrause commented Aug 1, 2024

Code looks okay and I've tested the modified feature in my development system.

One thing needs to be considered: Once the changes in this PR are applied, already existing statically cached content (that still contains HTML) is unsuitable to be rendered with the updated style sheets. It will look something like this:

Screenshot 2024-08-01 at 12-52-28 Habilitation

After any change, the newly created statically cached content (that now contains XML) works fine. It will look something like this:

image

Since this PR targets the LTS branch, some migration strategy has to be devised and implemented. Ideally, one, that is going to be performed automatically.

What are your plans here?

Once a migration strategy has been devised and implemented, I will approve this PR.

@Possommi
Copy link
Contributor Author

Possommi commented Aug 1, 2024

Code looks okay and I've tested the modified feature in my development system.
[...]
Since this PR targets the LTS branch, some migration strategy has to be devised and implemented. Ideally, one, that is going to be performed automatically.

What are your plans here?

Once a migration strategy has been devised and implemented, I will approve this PR.

[1] Executing repair metadata search of type mods would update all static content.

[2] Alternatively I could add a check in mods-metadata-page.xsl

<div id="modalFrame-body" class="modal-body">
  <xsl:choose>
    <xsl:when test="$originalContent/div[@id=$boxID]/table">
      <xsl:copy-of select="$originalContent/div[@id=$boxID]/*"/>
       ...
      <xsl:otherwise>     
        <xsl:apply-templates select="$originalContent/div[@id=$boxID]/*" mode="history-modal"/>
      ...
</div>

I do prefer [1] although [2] requires no additional user actions after upgrading to the new LTS.

@toKrause toKrause changed the title MIR-1343 Do not render mir-admindata-box static MIR-1343 Do not render mir-admindata-box staticly Aug 13, 2024
Copy link
Contributor

@toKrause toKrause left a comment

Choose a reason for hiding this comment

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

Version [1] would certainly be simpler and would be okay (if documented in the migration documentation), if this PR would target the main branch, but since this PR targets the LTS branch, there really should be no manual migration step.

So, version [2], if I understand it correctly, wold render an old static file in the old way and a new static file in the new way. I don't think that is a desirable solution, since it would be unclear users why the history (seemingly on a random basis) has a different behavior for different objects.

I think a solution should be invented, that automatically creates the missing/unfit static data for in the new way if no/old static data is found.

@Possommi Possommi requested a review from toKrause August 21, 2024 08:16
@kkrebs kkrebs merged commit 6d192e4 into 2023.06.x Aug 29, 2024
2 checks passed
@kkrebs kkrebs deleted the issues/MIR-1343-Do-not-render-mir-admindata-box-static branch August 29, 2024 09:13
yagee-de added a commit that referenced this pull request Sep 11, 2024
* 2023.06.x:
  fix github actions
  MIR-1347-allow to change the number of entries per page of the filebox with a property
  MIR-1346 Allow to add custom meta elements in mir-flatmir-layout.xsl
  MIR-1349 include ID correctly
  MIR-1348 Provide realm of user in link to profile
  Bump follow-redirects in /mir-webapp/src/main/vue/name-search (#981)
  Bump follow-redirects in /mir-webapp/src/main/vue/editor-tools (#982)
  Bump webpack-dev-middleware in /mir-webapp/src/main/vue/name-search (#985)
  Bump express in /mir-webapp/src/main/vue/editor-tools (#993)
  Bump express in /mir-webapp/src/main/vue/name-search (#994)
  Bump micromatch in /mir-webapp/src/main/vue/editor-tools (#1050)
  Bump webpack in /mir-webapp/src/main/vue/name-search (#1051)
  MIR-1336 Display realname of creator/modifier in mir-admin-box (#1031)
  MIR-1343 Do not render mir-admindata-box staticly (#1040)
  MIR-1344 update JavaScript dependencies
  Bump select2 from 4.0.6-rc.1 to 4.0.6 in /mir-module
  MIR-1338 fixed missing url encoding in solr query (#1033)
  MIR-1337 fix property to control display of real name (#1032)
  MIR-1339 Obtain ORCID from lobid in xEditor name search (#1034)
  Bump jsoup from 1.15.2 to 1.15.3
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

Successfully merging this pull request may close these issues.

3 participants